[{"id":27150,"web_url":"https://patchwork.libcamera.org/comment/27150/","msgid":"","date":"2023-05-29T08:02:18","subject":"Re: [libcamera-devel] [PATCH v6 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 Mon, May 22, 2023 at 08:35:08AM +0000, Harvey Yang via libcamera-devel wrote:\n> From: Cheng-Hao Yang \n\nThis is the same email address with 2 different names. Unless this is\nintentional could you change the authorship of the patch and use a\nsingle identity (git commit --amend --autor=\"...\")\n\n>\n> Add a helper function exportFrameBuffers in HeapAllocator to make it\n> easier to use.\n>\n> Signed-off-by: Harvey Yang \n> ---\n> include/libcamera/internal/heap_allocator.h | 9 +++\n> src/libcamera/heap_allocator.cpp | 62 +++++++++++++++++++++\n> 2 files changed, 71 insertions(+)\n>\n> diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h\n> index 92d4488a..92beaaa4 100644\n> --- a/include/libcamera/internal/heap_allocator.h\n> +++ b/include/libcamera/internal/heap_allocator.h\n> @@ -9,12 +9,16 @@\n> #pragma once\n>\n> #include \n> +#include \n>\n> #include \n>\n> namespace libcamera {\n>\n> +class Camera;\n> +class FrameBuffer;\n> class Heap;\n> +class Stream;\n>\n> class HeapAllocator\n> {\n> @@ -24,7 +28,12 @@ public:\n> \tbool isValid() const;\n> \tUniqueFD alloc(const char *name, std::size_t size);\n>\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t std::vector> *buffers);\n> +\n> private:\n> +\tstd::unique_ptr createBuffer(Stream *stream);\n> +\n> \tstd::unique_ptr heap_;\n> };\n>\n> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> index 8f99f590..e99cdbe5 100644\n> --- a/src/libcamera/heap_allocator.cpp\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -22,6 +22,12 @@\n>\n> #include \n>\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/formats.h\"\n> +\n> namespace libcamera {\n>\n> /*\n> @@ -227,4 +233,60 @@ 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> *buffers)\n> +{\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\t/** \\todo Support multiplanar allocations */\n> +\n> +\tfor (unsigned i = 0; i < count; ++i) {\n> +\t\tstd::unique_ptr 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 HeapAllocator::createBuffer(Stream *stream)\n> +{\n> +\tstd::vector planes;\n> +\n> +\tunsigned int frameSize = stream->configuration().frameSize;\n> +\tint pageSize = getpagesize();\n> +\t/** Allocation size need to be a direct multiple of |pageSize|. */\n\nNo need for /**, just use /*\n\n> +\tunsigned int numPage = (frameSize + pageSize - 1) / pageSize;\n> +\n> +\tUniqueFD fd = alloc(\"FrameBuffer\", numPage * pageSize);\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\ns/shared_fd/sharedFd/\n\n> +\tunsigned int offset = 0;\n> +\tfor (size_t i = 0; i < info.planes.size(); ++i) {\n> +\t\tunsigned int planeSize = info.planeSize(stream->configuration().size, i);\n> +\t\tif (planeSize == 0)\n> +\t\t\tcontinue;\n> +\n> +\t\t/** \\todo We don't support allocating buffers with a dedicated fd per plane */\n> +\t\tFrameBuffer::Plane plane;\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(planes);\n> +}\n> +\n\nOnly minors, the patch looks good. The series looks good as well,\nplease add documentation and fix all the minors and I think we can\ncollect it :)\n\nThanks\n j\n\n> } /* namespace libcamera */\n> --\n> 2.40.1.698.g37aff9b760-goog\n>","headers":{"Return-Path":"","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 61363C31E9\n\tfor ;\n\tMon, 29 May 2023 08:02:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6D6D626FC;\n\tMon, 29 May 2023 10:02:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD501626F8\n\tfor ;\n\tMon, 29 May 2023 10:02:20 +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 D16AF327;\n\tMon, 29 May 2023 10:02:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685347341;\n\tbh=hr0qMZa31KLjTRbZR67jB5VPGWECAurR52IJ+yMR3HY=;\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=B65xT+Olrv8qhQL8mUiQ7DgEdChCWd8Cnn7L1TtDuqv7rkgn+82cKHbRB1mNnyHHw\n\t2em4FWAYG00xqR6MNQqp6VvtHYHntLIXo1JNoB5Nems+3C9P5FBmnQnH3TIsP2VUya\n\tCnaNqDTEpqaosclS3gK6FBD/11rKcnOIbxV/0SIE7gXH7Nni7SEaNTWMTu79Yz7RE+\n\tQO2aAkfuCSt5Q2MJWjtrR9hAL5ZtUfTd9/VZDULuYDJ22urxXQbRX146BJlHhKer/H\n\tBR8ae1kxqWjdtZ5PQib1ufgZVvsXQth+zOZxAbAnCU0yFW2FhXRrawmp2bIBCG2Q+5\n\tAlzweHJvARrjA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685347321;\n\tbh=hr0qMZa31KLjTRbZR67jB5VPGWECAurR52IJ+yMR3HY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TScqUbA6TQHiDtKhBHAyZQ3/pbUFFxE8PisIUmDA+0qiFt51F+4oRdCxcflA3c2tQ\n\tLTOwV/l/WhQxks+AHrS91BYGGLqVgQ4jxKxXGH2GBmUi4vDZnN5/uvXeDQ7wfsobb1\n\ttzagPiNrByTN4pv2adyoj+9deMSqkk/+FDYEmUfE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TScqUbA6\"; dkim-atps=neutral","Date":"Mon, 29 May 2023 10:02:18 +0200","To":"Harvey Yang ","Message-ID":"","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-4-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230522083546.2465448-4-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v6 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27166,"web_url":"https://patchwork.libcamera.org/comment/27166/","msgid":"<4058db49-95a8-389b-23cc-81f0240d0573@ideasonboard.com>","date":"2023-05-30T05:25:29","subject":"Re: [libcamera-devel] [PATCH v6 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Harvey,\n\nThank you for the patch.\n\nOn 5/22/23 2:05 PM, Harvey Yang via libcamera-devel wrote:\n> From: Cheng-Hao Yang \n>\n> Add a helper function exportFrameBuffers in HeapAllocator to make it\n> easier to use.\n>\n> Signed-off-by: Harvey Yang \n> ---\n> include/libcamera/internal/heap_allocator.h | 9 +++\n> src/libcamera/heap_allocator.cpp | 62 +++++++++++++++++++++\n> 2 files changed, 71 insertions(+)\n>\n> diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h\n> index 92d4488a..92beaaa4 100644\n> --- a/include/libcamera/internal/heap_allocator.h\n> +++ b/include/libcamera/internal/heap_allocator.h\n> @@ -9,12 +9,16 @@\n> #pragma once\n> \n> #include \n> +#include \n> \n> #include \n> \n> namespace libcamera {\n> \n> +class Camera;\n> +class FrameBuffer;\n> class Heap;\n> +class Stream;\n> \n> class HeapAllocator\n> {\n> @@ -24,7 +28,12 @@ public:\n> \tbool isValid() const;\n> \tUniqueFD alloc(const char *name, std::size_t size);\n> \n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t std::vector> *buffers);\n> +\n\nCan this be made to accept references instead of pointers? I believe \nevery argument is non-nullable here...\n> private:\n> +\tstd::unique_ptr createBuffer(Stream *stream);\n> +\n> \tstd::unique_ptr heap_;\n> };\n> \n> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> index 8f99f590..e99cdbe5 100644\n> --- a/src/libcamera/heap_allocator.cpp\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -22,6 +22,12 @@\n> \n> #include \n> \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/formats.h\"\n> +\n> namespace libcamera {\n> \n> /*\n> @@ -227,4 +233,60 @@ 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> *buffers)\n> +{\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\t/** \\todo Support multiplanar allocations */\n> +\n> +\tfor (unsigned i = 0; i < count; ++i) {\n> +\t\tstd::unique_ptr 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 HeapAllocator::createBuffer(Stream *stream)\n> +{\n> +\tstd::vector planes;\n> +\n> +\tunsigned int frameSize = stream->configuration().frameSize;\n> +\tint pageSize = getpagesize();\n> +\t/** Allocation size need to be a direct multiple of |pageSize|. */\n\n\t/* Allocation size need to be a direct multiple of pageSize. */\n\n> +\tunsigned int numPage = (frameSize + pageSize - 1) / pageSize;\n> +\n> +\tUniqueFD fd = alloc(\"FrameBuffer\", numPage * pageSize);\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 (size_t i = 0; i < info.planes.size(); ++i) {\n> +\t\tunsigned int planeSize = info.planeSize(stream->configuration().size, i);\n> +\t\tif (planeSize == 0)\n> +\t\t\tcontinue;\n> +\n> +\t\t/** \\todo We don't support allocating buffers with a dedicated fd per plane */\n\n\t\t/* \\todo We don't support allocating buffers with a dedicated fd per plane */\n\n> +\t\tFrameBuffer::Plane plane;\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(planes);\n> +}\n> +\n> } /* namespace libcamera */","headers":{"Return-Path":"","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 BE910C31E9\n\tfor ;\n\tTue, 30 May 2023 05:25:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A048626F9;\n\tTue, 30 May 2023 07:25:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 72F3D6038F\n\tfor ;\n\tTue, 30 May 2023 07:25:35 +0200 (CEST)","from [192.168.1.108] (unknown [103.86.18.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7118327C;\n\tTue, 30 May 2023 07:25:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685424337;\n\tbh=vq0ji4V4zcu8fGzUzv2g+DaxNE7eUr1Sde0S1Xukrl0=;\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:\n\tFrom;\n\tb=s4JzQ8NJVTLax1a2xgDydy/lJx7E8opgJoM/wRheM1jQn0Cbvpno14bFK/d9gNq47\n\tUGyq5MCjTKCMLBtxblaMk3IT1GuZYQrNrr48290RkTP5lwq3xINy4VENn7OeLUVoj9\n\tuJWhggewvlYfA1xs0pP4WKzoF3LlWefnLXdWQcrcN6C1qGkqDGZwHC6ViSJtAD8p60\n\t3+PUrxUoD0UZpooQcH8n+ajVr+fpfrFTXGyClDt02q/JCXkZ9DyYh5ByvJUIA+J5ny\n\txvEMAmWF7uU0A6t3iq5ccfQcQsTDYtnKvu0lJnEBwmkXTwaIBjMFWU37zj50Oefzy7\n\tSjLV6ZB8C5OxQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685424314;\n\tbh=vq0ji4V4zcu8fGzUzv2g+DaxNE7eUr1Sde0S1Xukrl0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Ev0LBmvxdo0eFnd6JDPlzX9bPeNbGpGPbK65m5TlvRoq2v4emuIc4nHoieFTyVzXG\n\t70RgTRy4sWk8HeHWWzZKJ7p75U6mVNTroLF14N2cNaLtp8d4kkXrIiPhr6WOQYh24l\n\tdtRRXPv0OAUfkuhs6DjduKmuDRn9AtFOBLp2AZks="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Ev0LBmvx\"; dkim-atps=neutral","Message-ID":"<4058db49-95a8-389b-23cc-81f0240d0573@ideasonboard.com>","Date":"Tue, 30 May 2023 10:55:29 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-4-chenghaoyang@google.com>","Content-Language":"en-US","In-Reply-To":"<20230522083546.2465448-4-chenghaoyang@google.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v6 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27170,"web_url":"https://patchwork.libcamera.org/comment/27170/","msgid":"<20230530065608.GC6865@pendragon.ideasonboard.com>","date":"2023-05-30T06:56:08","subject":"Re: [libcamera-devel] [PATCH v6 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, May 29, 2023 at 10:02:18AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Hi Harvey\n> \n> On Mon, May 22, 2023 at 08:35:08AM +0000, Harvey Yang via libcamera-devel wrote:\n> > From: Cheng-Hao Yang \n> \n> This is the same email address with 2 different names. Unless this is\n> intentional could you change the authorship of the patch and use a\n> single identity (git commit --amend --autor=\"...\")\n> \n> > Add a helper function exportFrameBuffers in HeapAllocator to make it\n> > easier to use.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > include/libcamera/internal/heap_allocator.h | 9 +++\n> > src/libcamera/heap_allocator.cpp | 62 +++++++++++++++++++++\n> > 2 files changed, 71 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h\n> > index 92d4488a..92beaaa4 100644\n> > --- a/include/libcamera/internal/heap_allocator.h\n> > +++ b/include/libcamera/internal/heap_allocator.h\n> > @@ -9,12 +9,16 @@\n> > #pragma once\n> >\n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > namespace libcamera {\n> >\n> > +class Camera;\n> > +class FrameBuffer;\n> > class Heap;\n> > +class Stream;\n> >\n> > class HeapAllocator\n> > {\n> > @@ -24,7 +28,12 @@ public:\n> > \tbool isValid() const;\n> > \tUniqueFD alloc(const char *name, std::size_t size);\n> >\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\t\t\t std::vector> *buffers);\n> > +\n> > private:\n> > +\tstd::unique_ptr createBuffer(Stream *stream);\n> > +\n> > \tstd::unique_ptr heap_;\n> > };\n> >\n> > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> > index 8f99f590..e99cdbe5 100644\n> > --- a/src/libcamera/heap_allocator.cpp\n> > +++ b/src/libcamera/heap_allocator.cpp\n> > @@ -22,6 +22,12 @@\n> >\n> > #include \n> >\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/formats.h\"\n> > +\n> > namespace libcamera {\n> >\n> > /*\n> > @@ -227,4 +233,60 @@ 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> *buffers)\n\nWhy do you pass a camera pointer to this function if it's unused ?\n\n> > +{\n> > +\tunsigned int count = stream->configuration().bufferCount;\n\nYou're creating a tight dependency between the allocator and the stream\nhere, as you require the stream to have already been configured, and\nrely on the current configuration. Passing a stream configuration\nexplicitly would be better to make the allocator more generic.\n\n> > +\n> > +\t/** \\todo Support multiplanar allocations */\n\nI'd like to see this being addressed already, as it's a key requirement\nthat will show whether the API design is good or not.\n\n> > +\n> > +\tfor (unsigned i = 0; i < count; ++i) {\n> > +\t\tstd::unique_ptr 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 HeapAllocator::createBuffer(Stream *stream)\n> > +{\n> > +\tstd::vector planes;\n> > +\n> > +\tunsigned int frameSize = stream->configuration().frameSize;\n> > +\tint pageSize = getpagesize();\n> > +\t/** Allocation size need to be a direct multiple of |pageSize|. */\n> \n> No need for /**, just use /*\n> \n> > +\tunsigned int numPage = (frameSize + pageSize - 1) / pageSize;\n\nWhat are the pros and cons for rounding it up to the page size here\ncompared to the alloc() function ? Does the kernel require a rounded\nsize, of will it round internally ?\n\n> > +\n> > +\tUniqueFD fd = alloc(\"FrameBuffer\", numPage * pageSize);\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> \n> s/shared_fd/sharedFd/\n> \n> > +\tunsigned int offset = 0;\n> > +\tfor (size_t i = 0; i < info.planes.size(); ++i) {\n> > +\t\tunsigned int planeSize = info.planeSize(stream->configuration().size, i);\n\nThis doesn't take line stride into account.\n\n> > +\t\tif (planeSize == 0)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\t/** \\todo We don't support allocating buffers with a dedicated fd per plane */\n\nThis then makes the buffer incompatible with V4L2, that seems to be a\nbad issue for a generic allocator in libcamera.\n\n> > +\t\tFrameBuffer::Plane plane;\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(planes);\n> > +}\n> > +\n> \n> Only minors, the patch looks good. The series looks good as well,\n> please add documentation and fix all the minors and I think we can\n> collect it :)\n\nI'm afraid I disagree. See my comment in patch 2/3, in addition to the\nabove.\n\n> > } /* namespace libcamera */","headers":{"Return-Path":"","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 619C3C31E9\n\tfor ;\n\tTue, 30 May 2023 06:56:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C59B1626FA;\n\tTue, 30 May 2023 08:56:09 +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 B81C7626F7\n\tfor ;\n\tTue, 30 May 2023 08:56:08 +0200 (CEST)","from pendragon.ideasonboard.com (om126205206011.34.openmobile.ne.jp\n\t[126.205.206.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E16F3E4;\n\tTue, 30 May 2023 08:55:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685429769;\n\tbh=BWIPgS6H7MiBOSIu3BaCUBfNvUSksKp/Y34RhCPyoZ0=;\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=iS02ZYkGKSwsMoOZ6V0p+tGrIS9ljC1f0Lgm7JYDkdWWPxBZad8naWF09+qOR4ZQV\n\tip1ztaCRrjwSRRIebMgNZb+/jcFg6/iQ/9yBEK1/ONwBmk5AKZl7dKbDkKqKzDKq+V\n\t+0KEYbKITeGIj0R9ZdSeWyLF06gTWqy3nmIfFPLL2ZZuWNX/g9bN8Fozw4MxbJV2LR\n\tGLjaOzJRV3UbtgBA0Kdz2EIvmLl08MsSc2qLirHrqyHia3abchF1yCLJ1CHZdlC/6/\n\tRuo95PKIIz16wYt3DEwNkvr6C4qKXdgRJYXD4Or3nQbvv/hRHX+lVuDZpzO7T3KgLB\n\tdhUqI8UnPLmMw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685429748;\n\tbh=BWIPgS6H7MiBOSIu3BaCUBfNvUSksKp/Y34RhCPyoZ0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fY65VkMvXS8SrwVezeCvPHe15jtX4Cm/IHkOQN9pEjgAN5nGq8w+mCTgs8+yR2x7h\n\tfKInHWHga90pm2bUSa/WXZBhPhXOwyHLtJYDxk77gH9maJCw9/nf8Za4AgJiYL2yI1\n\tkUOMEaClf0iBajVyokMQ56Fryz+TqGca6UpzOu2U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fY65VkMv\"; dkim-atps=neutral","Date":"Tue, 30 May 2023 09:56:08 +0300","To":"Jacopo Mondi ","Message-ID":"<20230530065608.GC6865@pendragon.ideasonboard.com>","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-4-chenghaoyang@google.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27652,"web_url":"https://patchwork.libcamera.org/comment/27652/","msgid":"","date":"2023-08-02T07:08:05","subject":"Re: [libcamera-devel] [PATCH v6 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 Laurent,\n\nOn Tue, May 30, 2023 at 2:56 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Mon, May 29, 2023 at 10:02:18AM +0200, Jacopo Mondi via libcamera-devel\n> wrote:\n> > Hi Harvey\n> >\n> > On Mon, May 22, 2023 at 08:35:08AM +0000, Harvey Yang via\n> libcamera-devel wrote:\n> > > From: Cheng-Hao Yang \n> >\n> > This is the same email address with 2 different names. Unless this is\n> > intentional could you change the authorship of the patch and use a\n> > single identity (git commit --amend --autor=\"...\")\n> >\n> > > Add a helper function exportFrameBuffers in HeapAllocator to make it\n> > > easier to use.\n> > >\n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > include/libcamera/internal/heap_allocator.h | 9 +++\n> > > src/libcamera/heap_allocator.cpp | 62 +++++++++++++++++++++\n> > > 2 files changed, 71 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/heap_allocator.h\n> b/include/libcamera/internal/heap_allocator.h\n> > > index 92d4488a..92beaaa4 100644\n> > > --- a/include/libcamera/internal/heap_allocator.h\n> > > +++ b/include/libcamera/internal/heap_allocator.h\n> > > @@ -9,12 +9,16 @@\n> > > #pragma once\n> > >\n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > namespace libcamera {\n> > >\n> > > +class Camera;\n> > > +class FrameBuffer;\n> > > class Heap;\n> > > +class Stream;\n> > >\n> > > class HeapAllocator\n> > > {\n> > > @@ -24,7 +28,12 @@ public:\n> > > bool isValid() const;\n> > > UniqueFD alloc(const char *name, std::size_t size);\n> > >\n> > > + int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > + std::vector>\n> *buffers);\n> > > +\n> > > private:\n> > > + std::unique_ptr createBuffer(Stream *stream);\n> > > +\n> > > std::unique_ptr heap_;\n> > > };\n> > >\n> > > diff --git a/src/libcamera/heap_allocator.cpp\n> b/src/libcamera/heap_allocator.cpp\n> > > index 8f99f590..e99cdbe5 100644\n> > > --- a/src/libcamera/heap_allocator.cpp\n> > > +++ b/src/libcamera/heap_allocator.cpp\n> > > @@ -22,6 +22,12 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/formats.h\"\n> > > +\n> > > namespace libcamera {\n> > >\n> > > /*\n> > > @@ -227,4 +233,60 @@ 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, Stream *stream,\n> > > +\n> std::vector> *buffers)\n>\n> Why do you pass a camera pointer to this function if it's unused ?\n>\n>\nRemoved.\n\n\n> > > +{\n> > > + unsigned int count = stream->configuration().bufferCount;\n>\n> You're creating a tight dependency between the allocator and the stream\n> here, as you require the stream to have already been configured, and\n> rely on the current configuration. Passing a stream configuration\n> explicitly would be better to make the allocator more generic.\n>\n>\nGood point. Using `const StreamConfiguration&` directly.\n\n\n> > > +\n> > > + /** \\todo Support multiplanar allocations */\n>\n> I'd like to see this being addressed already, as it's a key requirement\n> that will show whether the API design is good or not.\n>\n>\nIIUC, this means the same thing as `a dedicated fd per plane`, right?\nLet me know if I misunderstood.\n\n\n> > > +\n> > > + for (unsigned i = 0; i < count; ++i) {\n> > > + std::unique_ptr 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 HeapAllocator::createBuffer(Stream\n> *stream)\n> > > +{\n> > > + std::vector planes;\n> > > +\n> > > + unsigned int frameSize = stream->configuration().frameSize;\n> > > + int pageSize = getpagesize();\n> > > + /** Allocation size need to be a direct multiple of |pageSize|. */\n> >\n> > No need for /**, just use /*\n> >\n> > > + unsigned int numPage = (frameSize + pageSize - 1) / pageSize;\n>\n> What are the pros and cons for rounding it up to the page size here\n> compared to the alloc() function ? Does the kernel require a rounded\n> size, of will it round internally ?\n>\n>\nMoved to `HeapAllocator::alloc` to prevent misuse in other components.\nYeah, the kernel requires a rounded size or it fails (in my environment with\nudmabuf).\n\n> > +\n> > > + UniqueFD fd = alloc(\"FrameBuffer\", numPage * pageSize);\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> >\n> > s/shared_fd/sharedFd/\n> >\n> > > + unsigned int offset = 0;\n> > > + for (size_t i = 0; i < info.planes.size(); ++i) {\n> > > + unsigned int planeSize =\n> info.planeSize(stream->configuration().size, i);\n>\n> This doesn't take line stride into account.\n>\n>\nIIUC, `PixelFormatInfo::planeSize` considers stride in the\nimplementation...?\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.cpp#n1022\n\n\n\n> > > + if (planeSize == 0)\n> > > + continue;\n> > > +\n> > > + /** \\todo We don't support allocating buffers with a\n> dedicated fd per plane */\n>\n> This then makes the buffer incompatible with V4L2, that seems to be a\n> bad issue for a generic allocator in libcamera.\n>\n>\nUpdated. Please check if it makes sense.\n\n\n> > > + FrameBuffer::Plane plane;\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(planes);\n> > > +}\n> > > +\n> >\n> > Only minors, the patch looks good. The series looks good as well,\n> > please add documentation and fix all the minors and I think we can\n> > collect it :)\n>\n> I'm afraid I disagree. See my comment in patch 2/3, in addition to the\n> above.\n>\n> > > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","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 DA9A9BDB13\n\tfor ;\n\tWed, 2 Aug 2023 07:08:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B797627E7;\n\tWed, 2 Aug 2023 09:08:19 +0200 (CEST)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B186627E6\n\tfor ;\n\tWed, 2 Aug 2023 09:08:17 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2b9bf52cd08so100965761fa.2\n\tfor ;\n\tWed, 02 Aug 2023 00:08:17 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690960099;\n\tbh=mwt6svGry7MjY6HDKHF1N2zaL1kaffTloepWumGiuyA=;\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=MF4G16g7XV2qbZn2PozOYijslisCajApeF6ob2csbP05BogWpQ04LmmRYmvozgv97\n\tecuRy30sRbhnXSnOwz1C4+cGVDio2NCT7lFGF0NqhVSXYc1wYw3VsiACYujcbJsJb/\n\tNFUtLcwQazsnRJVDX6lmREhwPgPA0nc9q+HjyByjV5IrUFLXr67T0dwrIV1cyFLvMe\n\tgtdp2bL2KdBUNPmC3YtTnsodG2YuJ3CSpQArtJrPJQuNJyDTiA+0nvYqbXeGujkG2P\n\tW45sRrN/QNKAaVOrd3LhPoYzWSpMEFHi4gLivAE9w/Vv7QM0Ykt5mPw3/srN3y6MeP\n\topieTTcIuHAiQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1690960096; x=1691564896;\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=OT081d67AMQx9LUnFWlNMxsqFaAjxei23Hb+BaNKCRM=;\n\tb=TX1+zMEY++wJe+vfbevUj183Q0zlaL/JO1AYhYDSxX1jOviy/+lRxFiA182zN0r8La\n\t2fm8EU2nH1HpDG8jnPqJjcacOrouqLzBzEEOFUzqvwblHzqQB6CvQ4CkVDWr2I5Ra7VV\n\tSCyteVvU2okHim2pH7FrdhVwVADQI9KTRq04Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"TX1+zMEY\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690960096; x=1691564896;\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=OT081d67AMQx9LUnFWlNMxsqFaAjxei23Hb+BaNKCRM=;\n\tb=OO5PRTWbMX1EmnNTF8zyv9ZABopTDoM8Y3+p5ZxgmvXZ850pwQXS8BaxGq4eydOS1z\n\tbJb5l/TUClw0IEJitW6dNeokdWXWP1PNoiCjJTTdVXKPYPFd4OXXPgzuw6D/WJRkWtp2\n\t88/FozyxqckHI4izddhF2ERyE63Srif9ZeTp89NVtgbrqEar2fw0NoXnmxaduHO4iZU0\n\t/gLgWuq9surU3i9nKTFn3gNOgtJZLo1lZroC4zeRi+qgbgVqd37JLjhP+vJcdUQU9u1Q\n\tsQOQj5TiaAEsStkHARF59XU8w2sfO4+abUxApeH7xssTMjYgx0DoPgVQ7btfksVIM1jf\n\tF71A==","X-Gm-Message-State":"ABy/qLbXNwbxznU7WXgkzbHxnW/+HtwzRlzOBalMD/6lxKIP3vImkJFc\n\tfVsZ7RINYhL/wPMJKpIrcJM4X6iXiKPK35FAIyJRYA==","X-Google-Smtp-Source":"APBJJlEFCd+zcxYu+oefzMEfVUSc4fwlEZNq1E7E1A2wB8r9Oh0yJ6g19hmGOXyt9eiGcqs6Sh7qOajgui2rKlOLkZY=","X-Received":"by 2002:a2e:99c7:0:b0:2b6:fc80:c45f with SMTP id\n\tl7-20020a2e99c7000000b002b6fc80c45fmr3713472ljj.13.1690960096558;\n\tWed, 02 Aug 2023 00:08:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-4-chenghaoyang@google.com>\n\t\n\t<20230530065608.GC6865@pendragon.ideasonboard.com>","In-Reply-To":"<20230530065608.GC6865@pendragon.ideasonboard.com>","Date":"Wed, 2 Aug 2023 15:08:05 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000e2967c0601eb53e5\"","Subject":"Re: [libcamera-devel] [PATCH v6 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27654,"web_url":"https://patchwork.libcamera.org/comment/27654/","msgid":"<20230802200707.GC29718@pendragon.ideasonboard.com>","date":"2023-08-02T20:07:07","subject":"Re: [libcamera-devel] [PATCH v6 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 02, 2023 at 03:08:05PM +0800, Cheng-Hao Yang wrote:\n> On Tue, May 30, 2023 at 2:56 PM Laurent Pinchart wrote:\n> > On Mon, May 29, 2023 at 10:02:18AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > On Mon, May 22, 2023 at 08:35:08AM +0000, Harvey Yang via libcamera-devel wrote:\n> > > > From: Cheng-Hao Yang \n> > >\n> > > This is the same email address with 2 different names. Unless this is\n> > > intentional could you change the authorship of the patch and use a\n> > > single identity (git commit --amend --autor=\"...\")\n> > >\n> > > > Add a helper function exportFrameBuffers in HeapAllocator to make it\n> > > > easier to use.\n> > > >\n> > > > Signed-off-by: Harvey Yang \n> > > > ---\n> > > > include/libcamera/internal/heap_allocator.h | 9 +++\n> > > > src/libcamera/heap_allocator.cpp | 62 +++++++++++++++++++++\n> > > > 2 files changed, 71 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h\n> > > > index 92d4488a..92beaaa4 100644\n> > > > --- a/include/libcamera/internal/heap_allocator.h\n> > > > +++ b/include/libcamera/internal/heap_allocator.h\n> > > > @@ -9,12 +9,16 @@\n> > > > #pragma once\n> > > >\n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > +class Camera;\n> > > > +class FrameBuffer;\n> > > > class Heap;\n> > > > +class Stream;\n> > > >\n> > > > class HeapAllocator\n> > > > {\n> > > > @@ -24,7 +28,12 @@ public:\n> > > > bool isValid() const;\n> > > > UniqueFD alloc(const char *name, std::size_t size);\n> > > >\n> > > > + int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > > + std::vector> *buffers);\n> > > > +\n> > > > private:\n> > > > + std::unique_ptr createBuffer(Stream *stream);\n> > > > +\n> > > > std::unique_ptr heap_;\n> > > > };\n> > > >\n> > > > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> > > > index 8f99f590..e99cdbe5 100644\n> > > > --- a/src/libcamera/heap_allocator.cpp\n> > > > +++ b/src/libcamera/heap_allocator.cpp\n> > > > @@ -22,6 +22,12 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/formats.h\"\n> > > > +\n> > > > namespace libcamera {\n> > > >\n> > > > /*\n> > > > @@ -227,4 +233,60 @@ UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)\n> > > > return heap_->alloc(name, size);\n> > > > }\n> > > >\n> > > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> > > > +\t\t\t\t std::vector> *buffers)\n> >\n> > Why do you pass a camera pointer to this function if it's unused ?\n>\n> Removed.\n> \n> > > > +{\n> > > > + unsigned int count = stream->configuration().bufferCount;\n> >\n> > You're creating a tight dependency between the allocator and the stream\n> > here, as you require the stream to have already been configured, and\n> > rely on the current configuration. Passing a stream configuration\n> > explicitly would be better to make the allocator more generic.\n>\n> Good point. Using `const StreamConfiguration&` directly.\n> \n> > > > +\n> > > > + /** \\todo Support multiplanar allocations */\n> >\n> > I'd like to see this being addressed already, as it's a key requirement\n> > that will show whether the API design is good or not.\n>\n> IIUC, this means the same thing as `a dedicated fd per plane`, right?\n> Let me know if I misunderstood.\n\nThat's right, yes.\n\n> > > > +\n> > > > + for (unsigned i = 0; i < count; ++i) {\n> > > > + std::unique_ptr buffer = createBuffer(stream);\n> > > > + if (!buffer) {\n> > > > + LOG(HeapAllocator, Error) << \"Unable to create 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 HeapAllocator::createBuffer(Stream *stream)\n> > > > +{\n> > > > + std::vector planes;\n> > > > +\n> > > > + unsigned int frameSize = stream->configuration().frameSize;\n> > > > + int pageSize = getpagesize();\n> > > > + /** Allocation size need to be a direct multiple of |pageSize|. */\n> > >\n> > > No need for /**, just use /*\n> > >\n> > > > + unsigned int numPage = (frameSize + pageSize - 1) / pageSize;\n> >\n> > What are the pros and cons for rounding it up to the page size here\n> > compared to the alloc() function ? Does the kernel require a rounded\n> > size, of will it round internally ?\n>\n> Moved to `HeapAllocator::alloc` to prevent misuse in other components.\n> Yeah, the kernel requires a rounded size or it fails (in my environment with\n> udmabuf).\n> \n> > > +\n> > > > + UniqueFD fd = alloc(\"FrameBuffer\", numPage * pageSize);\n> > > > + if (!fd.isValid())\n> > > > + return nullptr;\n> > > > +\n> > > > + auto info = PixelFormatInfo::info(stream->configuration().pixelFormat);\n> > > > + SharedFD shared_fd(std::move(fd));\n> > >\n> > > s/shared_fd/sharedFd/\n> > >\n> > > > + unsigned int offset = 0;\n> > > > + for (size_t i = 0; i < info.planes.size(); ++i) {\n> > > > + unsigned int planeSize = info.planeSize(stream->configuration().size, i);\n> >\n> > This doesn't take line stride into account.\n>\n> IIUC, `PixelFormatInfo::planeSize` considers stride in the\n> implementation...?\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.cpp#n1022\n\nWhat I meant is that it doesn't take StreamConfiguration::stride into\naccount.\n\n> > > > + if (planeSize == 0)\n> > > > + continue;\n> > > > +\n> > > > + /** \\todo We don't support allocating buffers with a dedicated fd per plane */\n> >\n> > This then makes the buffer incompatible with V4L2, that seems to be a\n> > bad issue for a generic allocator in libcamera.\n>\n> Updated. Please check if it makes sense.\n> \n> > > > + FrameBuffer::Plane plane;\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(planes);\n> > > > +}\n> > > > +\n> > >\n> > > Only minors, the patch looks good. The series looks good as well,\n> > > please add documentation and fix all the minors and I think we can\n> > > collect it :)\n> >\n> > I'm afraid I disagree. See my comment in patch 2/3, in addition to the\n> > above.\n> >\n> > > > } /* namespace libcamera */","headers":{"Return-Path":"","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 BA214BDCBF\n\tfor ;\n\tWed, 2 Aug 2023 20:07:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B3A6627EA;\n\tWed, 2 Aug 2023 22:07:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09464627E7\n\tfor ;\n\tWed, 2 Aug 2023 22:07:03 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CEDE69CA;\n\tWed, 2 Aug 2023 22:05:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1691006825;\n\tbh=qgDAt4dIZsgtnvigIuVQ9HL2S+Co16gv9JpbocO0gIA=;\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=knpU3I+pNB8g3ZJn3CGA/6KaPS9VPDaMRCd8TN+U0efeJe9OJA2rxv+Yn1FMJ8wWk\n\tDSM/0C6kZyaLrZr2uRK8Zwiy434lrGUq1RKbWN8ohTOfwubLM0/Vzp3M/ynP03q9qk\n\t8mMvDHYA9zpAV9QgIOCLHvUm2slo0cPOuxxwahRl43+VKUeV2t1/kDdeKOdn5wc/8T\n\tFZQXV1tQ8KMQ8RUz3b7WF6ibaqlYIC+gxBcfCyGzqmz9shXCkBTXsP2HFJ8P9nabSW\n\tYfcdLlaLeJ4YZyHN6ickGitTO/O0DC1otpLRFCrupEHVDMB2DB6mk54PFVd8je1+R5\n\tk9w5PgiHdKFTA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1691006759;\n\tbh=qgDAt4dIZsgtnvigIuVQ9HL2S+Co16gv9JpbocO0gIA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SUv0Gs8krIzARtjghGSkFt/1Yg76lx9f+2/vxWtRVkef6ZCaJx+sbh3je9Icet2p6\n\tTNFS3aaD42fI8DI01+N8CtLvfAl186uK4YoRW0UZYN+jcEbU4heDGNUMh033EhfB3M\n\todjaQPGQZlzaBVRC6+cOdapjkE6pl9pJEZeqOQgg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SUv0Gs8k\"; dkim-atps=neutral","Date":"Wed, 2 Aug 2023 23:07:07 +0300","To":"Cheng-Hao Yang ","Message-ID":"<20230802200707.GC29718@pendragon.ideasonboard.com>","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-4-chenghaoyang@google.com>\n\t\n\t<20230530065608.GC6865@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]