[{"id":30559,"web_url":"https://patchwork.libcamera.org/comment/30559/","msgid":"<20240803202049.GH3319@pendragon.ideasonboard.com>","date":"2024-08-03T20:20:49","subject":"Re: [PATCH v7 1/7] libcamera: add\n\tDmaBufAllocation::exportFrameBuffers()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Chenghao,\n\nThank you for the patch.\n\nOn Thu, Aug 01, 2024 at 07:30:57AM +0000, Harvey Yang wrote:\n> Add a helper function exportFrameBuffers in DmaBufAllocator to make it\n> easier to use.\n> \n> It'll be used in Virtual Pipeline Handler specifically.\n> \n> Signed-off-by: Harvey Yang \n> ---\n> .../libcamera/internal/dma_buf_allocator.h | 10 +++\n> src/libcamera/dma_buf_allocator.cpp | 68 ++++++++++++++++++-\n> 2 files changed, 76 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n> index 36ec1696..dd2cc237 100644\n> --- a/include/libcamera/internal/dma_buf_allocator.h\n> +++ b/include/libcamera/internal/dma_buf_allocator.h\n> @@ -8,12 +8,16 @@\n> #pragma once\n> \n> #include \n> +#include \n> \n> #include \n> #include \n> \n> namespace libcamera {\n> \n> +class FrameBuffer;\n> +struct StreamConfiguration;\n> +\n> class DmaBufAllocator\n> {\n> public:\n> @@ -30,7 +34,13 @@ public:\n> \tbool isValid() const { return providerHandle_.isValid(); }\n> \tUniqueFD alloc(const char *name, std::size_t size);\n> \n> +\tint exportFrameBuffers(\n> +\t\tconst StreamConfiguration &config,\n> +\t\tstd::vector> *buffers);\n\nI wonder if the DmaBufAllocator class is really the best place to\nimplement this. It would help reviewing how generic the implementation\nis if we had two users as part of this series. Maybe converting the soft\nISP to the new function could be such a second user.\n\nThe part that bothers me the most is, I think, usage of\nStreamConfiguration in this function. Maybe the createBuffer() function\ncould be kept, and be passed a pixel format and a size, whie the\nexportFrameBuffers() could be moved to the pipeline handler.\n\n> +\n> private:\n> +\tstd::unique_ptr createBuffer(const StreamConfiguration &config);\n> +\n> \tUniqueFD allocFromHeap(const char *name, std::size_t size);\n> \tUniqueFD allocFromUDmaBuf(const char *name, std::size_t size);\n> \tUniqueFD providerHandle_;\n> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> index c06eca7d..6b406880 100644\n> --- a/src/libcamera/dma_buf_allocator.cpp\n> +++ b/src/libcamera/dma_buf_allocator.cpp\n> @@ -23,6 +23,11 @@\n> \n> #include \n> \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/formats.h\"\n> +\n> /**\n> * \\file dma_buf_allocator.cpp\n> * \\brief dma-buf allocator\n> @@ -130,8 +135,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;\n> /* uClibc doesn't provide the file sealing API. */\n> #ifndef __DOXYGEN__\n> #if not HAVE_FILE_SEALS\n> -#define F_ADD_SEALS\t\t1033\n> -#define F_SEAL_SHRINK\t\t0x0002\n> +#define F_ADD_SEALS 1033\n> +#define F_SEAL_SHRINK 0x0002\n\nUnrelated change.\n\n> #endif\n> #endif\n> \n> @@ -243,4 +248,63 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)\n> \t\treturn allocFromHeap(name, size);\n> }\n> \n> +/**\n> + * \\brief Allocate and export buffers for \\a stream from the DmaBufAllocator\n> + * \\param[in] config The config of the stream to allocate buffers for\n> + * \\param[out] buffers Array of buffers successfully allocated\n> + *\n> + * Allocates buffers for a stream from the DmaBufAllocator. It's a helper\n> + * function that'll be used in PipelineHandler::exportFrameBuffers().\n> + *\n> + * \\return The number of allocated buffers on success or a negative error code\n> + * otherwise\n> + */\n> +int DmaBufAllocator::exportFrameBuffers(\n> +\tconst StreamConfiguration &config,\n> +\tstd::vector> *buffers)\n> +{\n> +\tunsigned int count = config.bufferCount;\n> +\n> +\tfor (unsigned i = 0; i < count; ++i) {\n> +\t\tstd::unique_ptr buffer = createBuffer(config);\n> +\t\tif (!buffer) {\n> +\t\t\tLOG(DmaBufAllocator, 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 DmaBufAllocator::createBuffer(\n> +\tconst StreamConfiguration &config)\n> +{\n> +\tstd::vector planes;\n> +\n> +\tauto info = PixelFormatInfo::info(config.pixelFormat);\n> +\tfor (size_t i = 0; i < info.planes.size(); ++i) {\n> +\t\tunsigned int planeSize = info.planeSize(config.size, i);\n> +\t\tif (planeSize == 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tUniqueFD fd = alloc(\"FrameBuffer\", planeSize);\n> +\t\tif (!fd.isValid())\n> +\t\t\treturn nullptr;\n> +\n> +\t\tSharedFD sharedFd(std::move(fd));\n> +\n> +\t\tFrameBuffer::Plane plane;\n> +\t\tplane.fd = sharedFd;\n\n\t\tplane.fd = SharedFD(std::move(fd));\n\t\nand drop the sharedFd variable.\n\n> +\t\tplane.offset = 0;\n> +\t\tplane.length = planeSize;\n> +\t\tplanes.push_back(std::move(plane));\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 478E4C323E\n\tfor ;\n\tSat, 3 Aug 2024 20:21:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58E096336E;\n\tSat, 3 Aug 2024 22:21:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 970BF6336E\n\tfor ;\n\tSat, 3 Aug 2024 22:21:11 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E3DB2C5;\n\tSat, 3 Aug 2024 22:20:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hOaM7IAD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722716421;\n\tbh=eYuzsSAnwsVHGqBIap/pLkd3hhpQtszxhk19/Jn4xHk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hOaM7IADfkn9MVZXV5qf6TPxE54MAEk9LD49G/FKHkswJeas2Bfje79qJ8FJPj2/z\n\twPF5QPUt/Gt2NF2lOOe32FiIEj5gBqUz6wgFjPci/x31Idq8IYRc570QHgmwcCa6m0\n\tYlueduUObi65n5uYR9+S51OzMyPipNZ+RcHJ/o6E=","Date":"Sat, 3 Aug 2024 23:20:49 +0300","From":"Laurent Pinchart ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Subject":"Re: [PATCH v7 1/7] libcamera: add\n\tDmaBufAllocation::exportFrameBuffers()","Message-ID":"<20240803202049.GH3319@pendragon.ideasonboard.com>","References":"<20240801073339.4061027-1-chenghaoyang@google.com>\n\t<20240801073339.4061027-2-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240801073339.4061027-2-chenghaoyang@google.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30596,"web_url":"https://patchwork.libcamera.org/comment/30596/","msgid":"","date":"2024-08-05T14:00:25","subject":"Re: [PATCH v7 1/7] libcamera: add\n\tDmaBufAllocation::exportFrameBuffers()","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nThanks for the review. Updated in v8.\n\nOn Sat, Aug 3, 2024 at 10:21 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Chenghao,\n>\n> Thank you for the patch.\n>\n> On Thu, Aug 01, 2024 at 07:30:57AM +0000, Harvey Yang wrote:\n> > Add a helper function exportFrameBuffers in DmaBufAllocator to make it\n> > easier to use.\n> >\n> > It'll be used in Virtual Pipeline Handler specifically.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > .../libcamera/internal/dma_buf_allocator.h | 10 +++\n> > src/libcamera/dma_buf_allocator.cpp | 68 ++++++++++++++++++-\n> > 2 files changed, 76 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/dma_buf_allocator.h\n> b/include/libcamera/internal/dma_buf_allocator.h\n> > index 36ec1696..dd2cc237 100644\n> > --- a/include/libcamera/internal/dma_buf_allocator.h\n> > +++ b/include/libcamera/internal/dma_buf_allocator.h\n> > @@ -8,12 +8,16 @@\n> > #pragma once\n> >\n> > #include \n> > +#include \n> >\n> > #include \n> > #include \n> >\n> > namespace libcamera {\n> >\n> > +class FrameBuffer;\n> > +struct StreamConfiguration;\n> > +\n> > class DmaBufAllocator\n> > {\n> > public:\n> > @@ -30,7 +34,13 @@ public:\n> > bool isValid() const { return providerHandle_.isValid(); }\n> > UniqueFD alloc(const char *name, std::size_t size);\n> >\n> > + int exportFrameBuffers(\n> > + const StreamConfiguration &config,\n> > + std::vector> *buffers);\n>\n> I wonder if the DmaBufAllocator class is really the best place to\n> implement this. It would help reviewing how generic the implementation\n> is if we had two users as part of this series. Maybe converting the soft\n> ISP to the new function could be such a second user.\n>\n>\nRight, it's a good idea that we add the helper functions in a way that works\nfor soft ISP as well.\n\n\n> The part that bothers me the most is, I think, usage of\n> StreamConfiguration in this function. Maybe the createBuffer() function\n> could be kept, and be passed a pixel format and a size, whie the\n> exportFrameBuffers() could be moved to the pipeline handler.\n>\n>\nHmm, as |SoftwareIsp::exportFrames| only uses `debayer_->frameSize()`,\nI updated the DmaBufAllocator's helper function with `count` and a list of\n`frameSize`s. Please check if it's better.\n\n\n\n> > +\n> > private:\n> > + std::unique_ptr createBuffer(const\n> StreamConfiguration &config);\n> > +\n> > UniqueFD allocFromHeap(const char *name, std::size_t size);\n> > UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);\n> > UniqueFD providerHandle_;\n> > diff --git a/src/libcamera/dma_buf_allocator.cpp\n> b/src/libcamera/dma_buf_allocator.cpp\n> > index c06eca7d..6b406880 100644\n> > --- a/src/libcamera/dma_buf_allocator.cpp\n> > +++ b/src/libcamera/dma_buf_allocator.cpp\n> > @@ -23,6 +23,11 @@\n> >\n> > #include \n> >\n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/formats.h\"\n> > +\n> > /**\n> > * \\file dma_buf_allocator.cpp\n> > * \\brief dma-buf allocator\n> > @@ -130,8 +135,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;\n> > /* uClibc doesn't provide the file sealing API. */\n> > #ifndef __DOXYGEN__\n> > #if not HAVE_FILE_SEALS\n> > -#define F_ADD_SEALS 1033\n> > -#define F_SEAL_SHRINK 0x0002\n> > +#define F_ADD_SEALS 1033\n> > +#define F_SEAL_SHRINK 0x0002\n>\n> Unrelated change.\n>\n>\nSorry, my linter auto-corrected it. Removed.\n\n\n> > #endif\n> > #endif\n> >\n> > @@ -243,4 +248,63 @@ UniqueFD DmaBufAllocator::alloc(const char *name,\n> std::size_t size)\n> > return allocFromHeap(name, size);\n> > }\n> >\n> > +/**\n> > + * \\brief Allocate and export buffers for \\a stream from the\n> DmaBufAllocator\n> > + * \\param[in] config The config of the stream to allocate buffers for\n> > + * \\param[out] buffers Array of buffers successfully allocated\n> > + *\n> > + * Allocates buffers for a stream from the DmaBufAllocator. It's a\n> helper\n> > + * function that'll be used in PipelineHandler::exportFrameBuffers().\n> > + *\n> > + * \\return The number of allocated buffers on success or a negative\n> error code\n> > + * otherwise\n> > + */\n> > +int DmaBufAllocator::exportFrameBuffers(\n> > + const StreamConfiguration &config,\n> > + std::vector> *buffers)\n> > +{\n> > + unsigned int count = config.bufferCount;\n> > +\n> > + for (unsigned i = 0; i < count; ++i) {\n> > + std::unique_ptr buffer = createBuffer(config);\n> > + if (!buffer) {\n> > + LOG(DmaBufAllocator, 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 DmaBufAllocator::createBuffer(\n> > + const StreamConfiguration &config)\n> > +{\n> > + std::vector planes;\n> > +\n> > + auto info = PixelFormatInfo::info(config.pixelFormat);\n> > + for (size_t i = 0; i < info.planes.size(); ++i) {\n> > + unsigned int planeSize = info.planeSize(config.size, i);\n> > + if (planeSize == 0)\n> > + continue;\n> > +\n> > + UniqueFD fd = alloc(\"FrameBuffer\", planeSize);\n> > + if (!fd.isValid())\n> > + return nullptr;\n> > +\n> > + SharedFD sharedFd(std::move(fd));\n> > +\n> > + FrameBuffer::Plane plane;\n> > + plane.fd = sharedFd;\n>\n> plane.fd = SharedFD(std::move(fd));\n>\n> and drop the sharedFd variable.\n>\n>\nDone.\n\n\n> > + plane.offset = 0;\n> > + plane.length = planeSize;\n> > + planes.push_back(std::move(plane));\n> > + }\n> > +\n> > + return std::make_unique(planes);\n> > +}\n> > +\n> > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\nThanks!\nHarvey","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 74594C323E\n\tfor ;\n\tMon, 5 Aug 2024 14:00:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 936D763369;\n\tMon, 5 Aug 2024 16:00:39 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 788006195D\n\tfor ;\n\tMon, 5 Aug 2024 16:00:37 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2f189a2a841so30651fa.3\n\tfor ;\n\tMon, 05 Aug 2024 07:00:37 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"D7nQfJYI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1722866436; x=1723471236;\n\tdarn=lists.libcamera.org; \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=+MPR60V0p6hXykG/7v8ENNbMEIozP3ZGuMwlSRJosvY=;\n\tb=D7nQfJYI6UKjZzNU8Wdaio5l9/kP7SSrznDzSbJIXfUflnfiqdWD5cfIDRIJFG4uFk\n\t4s1P4LG1MbC2OvfnlNg43uu+K22pH5CQwHk4/6eJ1QbSv+VjQ9nEaL9Zv8hEiERintft\n\trmemO/by1k/sL5gM+ESJ3Vf14fpReSsR1nEX4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1722866436; x=1723471236;\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=+MPR60V0p6hXykG/7v8ENNbMEIozP3ZGuMwlSRJosvY=;\n\tb=bn6TuigaZip/DwSzlQNvhXrQ8u8ymIx+49x1W5DcCsknyUnTY+Na2lb/mJkBlKbXQ5\n\tWzpXP7MrhQfKuAYi752ouGNcRN8vourbO5OyJeuXIWd9zWOoPGMbEtOD6hjdFQlhm697\n\tIZCW22hoANCQETNW2mnGRGQ2S6d2DAxNuE8OwdKYhoLbbLe6ay6LdNHYC7DYvNOCRBz/\n\tsZSKIedS8pRNNii6+ekbvoRlobp55wVf3EeUTYyt4ZOICIottgxnHKKaUN1+9N8Ynvkm\n\trNd7hA9LyNw2Tnu+6e+ZwckH3/Xw/RzgrZkQSopPstkRZRxAXKkaQ1qZrSWx9AAVR1eb\n\tyZaA==","X-Gm-Message-State":"AOJu0YxIlsuQ8+7nuZhNONZ0oU/iSyz2KUfu2bwiAGMORVow/3INc6GK\n\twIG7wWK6/0spjXEew6zOivRgzWVovLP1zjaDw6nbd+Ba/k2ywHgD/EODnG9U4JbAMkuZsBTdY4j\n\t8wTQcx7PbY/lnd9yy5XfM5KDtYoaR2RzibNIo","X-Google-Smtp-Source":"AGHT+IGlWrlhqKmUcMhJkLJaci+KGKEZc+MfM1YY5sXHLquh9dWGZaRg34GvlWLA4e6qe1xFnDZcL0RHmZZ5qYfjyZE=","X-Received":"by 2002:a2e:9b88:0:b0:2ef:1b93:d2b6 with SMTP id\n\t38308e7fff4ca-2f15aa84f34mr78313911fa.8.1722866436346;\n\tMon, 05 Aug 2024 07:00:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20240801073339.4061027-1-chenghaoyang@google.com>\n\t<20240801073339.4061027-2-chenghaoyang@google.com>\n\t<20240803202049.GH3319@pendragon.ideasonboard.com>","In-Reply-To":"<20240803202049.GH3319@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Mon, 5 Aug 2024 16:00:25 +0200","Message-ID":"","Subject":"Re: [PATCH v7 1/7] libcamera: add\n\tDmaBufAllocation::exportFrameBuffers()","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Content-Type":"multipart/alternative; boundary=\"000000000000ef2ddd061ef019e7\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]