[{"id":30923,"web_url":"https://patchwork.libcamera.org/comment/30923/","msgid":"","date":"2024-08-27T14:13:11","subject":"Re: [PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()","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, Aug 20, 2024 at 04:23:32PM GMT, Harvey Yang wrote:\n> Add a helper function exportBuffers in DmaBufAllocator to make it easier\n> to use.\n>\n> It'll be used in Virtual Pipeline Handler and SoftwareIsp.\n>\n> Signed-off-by: Harvey Yang \n> ---\n> .../libcamera/internal/dma_buf_allocator.h | 12 ++++\n> src/libcamera/dma_buf_allocator.cpp | 64 ++++++++++++++++++-\n> 2 files changed, 74 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 36ec1696b..3a9b56b1c 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\nWhere do you use this ?\n\n> +\n> class DmaBufAllocator\n> {\n> public:\n> @@ -30,7 +34,15 @@ public:\n> \tbool isValid() const { return providerHandle_.isValid(); }\n> \tUniqueFD alloc(const char *name, std::size_t size);\n>\n> +\tint exportBuffers(\n\nweird indent\n\n\tint exportBuffers(std::size_t count,\n\t\t\t const std::vector &frameSize,\n\t\t\t std::vector> *buffers);\n\n> +\t\tstd::size_t count,\n\n#include \n\n> +\t\tstd::vector frameSize,\n> +\t\tstd::vector> *buffers);\n\n#include \n\n> +\n> private:\n> +\tstd::unique_ptr createBuffer(\n> +\t\tstd::string name, std::vector frameSizes);\n\n#include \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 c06eca7d0..2d88b8b2b 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\nNot used\n\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\n\n> #endif\n> #endif\n>\n> @@ -243,4 +248,59 @@ 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\nThere's no \\a stream\n\n> + * \\param[in] count The number of FrameBuffers required\n\n\"of requested FrameBuffers\" ?\n\n> + * \\param[in] frameSizes The sizes of planes in the FrameBuffer\n> + * \\param[out] buffers Array of buffers successfully allocated\n> + *\n> + * \\return The number of allocated buffers on success or a negative error code\n> + * otherwise\n> + */\n> +int DmaBufAllocator::exportBuffers(\n> +\tstd::size_t count,\n\nnit: This can be an unsigned int maybe\n\n> +\tstd::vector frameSizes,\n\ncan this be passed as a const reference ?\n\n> +\tstd::vector> *buffers)\n\nweird indentation, I think\n\nint DmaBufAllocator::exportBuffers(std::size_t count,\n\t\t\t\t const std::vector &frameSizes,\n\t\t\t\t std::vector> *buffers)\n\nwould do (some heretics consider 120 cols to be ok for libcamera, and\nnowadays it is accepted for linux as well !! )\n\n> +{\n> +\tfor (unsigned i = 0; i < count; ++i) {\n\nI wonder if we should be stricter when defining this interface: is\nthere a maximum number of buffers that can be allocated ? How many\nplanes can a buffer have ? (I don't see this addressed in our\nFrameBuffer class, so it can be left out from here too ?)\n\n> +\t\tstd::unique_ptr buffer =\n> +\t\t\tcreateBuffer(\"frame-\" + std::to_string(i), frameSizes);\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\nif you want to have all the requested buffers to be allocated, and do\nnot allow less than that, there's no point in returning count here, you\ncan return 0\n\n> +}\n> +\n> +std::unique_ptr DmaBufAllocator::createBuffer(\n> +\tstd::string name, std::vector frameSizes)\n\nOr\n\nstd::unique_ptr\nDmaBufAllocator::createBuffer(std::string name,\n\t\t\t const std::vector &frameSizes)\n\ncan frameSize be passed as a const reference ?\n\n\n> +{\n> +\tstd::vector planes;\n> +\n> +\tstd::size_t bufferSize = 0, offset = 0;\n> +\tfor (auto frameSize : frameSizes)\n> +\t\tbufferSize += frameSize;\n\nThis API allows unbounded size allocation, just sayin', maybe it's\nfine\n\n> +\n> +\tSharedFD fd(alloc(name.c_str(), bufferSize));\n> +\tif (!fd.isValid())\n> +\t\treturn nullptr;\n> +\n> +\tfor (auto frameSize : frameSizes) {\n> +\t\tFrameBuffer::Plane plane;\n> +\t\tplane.fd = fd;\n> +\t\tplane.offset = offset;\n> +\t\tplane.length = frameSize;\n> +\t\tplanes.push_back(std::move(plane));\n> +\t\toffset += plane.length;\n> +\t}\n\nWith a little implicit type casting, you can\n\n\tunsigned int offset = 0;\n\tfor (unsigned int frameSize : frameSizes) {\n\t\tplanes.emplace_back(FrameBuffer::Plane{fd, offset, frameSize});\n\t\toffset += frameSize;\n\t}\n\nBut it won't save you going a temporary object I'm afraid. I'm not\nsure it's actually any better, up to you.\n\n> +\n> +\treturn std::make_unique(planes);\n> +}\n> +\n> } /* namespace libcamera */\n> --\n> 2.46.0.184.g6999bdac58-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 EB505C323E\n\tfor ;\n\tTue, 27 Aug 2024 14:13:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A238863460;\n\tTue, 27 Aug 2024 16:13:18 +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 4223761901\n\tfor ;\n\tTue, 27 Aug 2024 16:13:16 +0200 (CEST)","from ideasonboard.com (mob-5-90-141-165.net.vodafone.it\n\t[5.90.141.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC41C6A6;\n\tTue, 27 Aug 2024 16:12:08 +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=\"gOMOaFbZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724767929;\n\tbh=c5nu6fDGxV78dn5JgjMroC/WOKw3icu2uFAB6+LBJYU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gOMOaFbZWYLIlFO/QrGSuWvTRS28PqLPSMVuZ71USOk+dEb4IDVl/Lr2j4cZNcI+a\n\thNOJqYqPLBqylhUtsQ2wEHjWyCLgDj6M5Lp82fh5Oy/IDWrMDsUryTGw/5y5SgltkW\n\tttLYr0arDI32EWm/MV7OdwpJNP+sTMsI/hAWIjtY=","Date":"Tue, 27 Aug 2024 16:13:11 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Subject":"Re: [PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()","Message-ID":"","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-2-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240820172202.526547-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":30927,"web_url":"https://patchwork.libcamera.org/comment/30927/","msgid":"","date":"2024-08-27T15:34:43","subject":"Re: [PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Jacopo for the review.\n\nThe corresponding updates are included in v9.1. Please check.\n\nOn Tue, Aug 27, 2024 at 4:13 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Tue, Aug 20, 2024 at 04:23:32PM GMT, Harvey Yang wrote:\n> > Add a helper function exportBuffers in DmaBufAllocator to make it easier\n> > to use.\n> >\n> > It'll be used in Virtual Pipeline Handler and SoftwareIsp.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > .../libcamera/internal/dma_buf_allocator.h | 12 ++++\n> > src/libcamera/dma_buf_allocator.cpp | 64 ++++++++++++++++++-\n> > 2 files changed, 74 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 36ec1696b..3a9b56b1c 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> Where do you use this ?\n>\n>\nRight, it's only used in the previous versions. Removed.\n\n\n> > +\n> > class DmaBufAllocator\n> > {\n> > public:\n> > @@ -30,7 +34,15 @@ public:\n> > bool isValid() const { return providerHandle_.isValid(); }\n> > UniqueFD alloc(const char *name, std::size_t size);\n> >\n> > + int exportBuffers(\n>\n> weird indent\n>\n> int exportBuffers(std::size_t count,\n> const std::vector &frameSize,\n> std::vector>\n> *buffers);\n>\n> Done\n\n\n> > + std::size_t count,\n>\n> #include \n>\nRemoved std::size_t.\n\n\n>\n> > + std::vector frameSize,\n> > + std::vector> *buffers);\n>\n> #include \n>\n> Done\n\n\n> > +\n> > private:\n> > + std::unique_ptr createBuffer(\n> > + std::string name, std::vector frameSizes);\n>\n> #include \n>\nDone\n\n\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 c06eca7d0..2d88b8b2b 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> Not used\n>\nRemoved\n\n\n>\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\n>\n> Removed the changes\n\n\n> > #endif\n> > #endif\n> >\n> > @@ -243,4 +248,59 @@ 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>\n> There's no \\a stream\n>\nRemoved.\n\n\n>\n> > + * \\param[in] count The number of FrameBuffers required\n>\n> \"of requested FrameBuffers\" ?\n>\nThanks! Adopted.\n\n\n>\n> > + * \\param[in] frameSizes The sizes of planes in the FrameBuffer\n> > + * \\param[out] buffers Array of buffers successfully allocated\n> > + *\n> > + * \\return The number of allocated buffers on success or a negative\n> error code\n> > + * otherwise\n> > + */\n> > +int DmaBufAllocator::exportBuffers(\n> > + std::size_t count,\n>\n> nit: This can be an unsigned int maybe\n>\nDone\n\n\n>\n> > + std::vector frameSizes,\n>\n> can this be passed as a const reference ?\n>\nDone\n\n\n>\n> > + std::vector> *buffers)\n>\n> weird indentation, I think\n>\n> int DmaBufAllocator::exportBuffers(std::size_t count,\n> const std::vector\n> &frameSizes,\n>\n> std::vector> *buffers)\n>\n> would do (some heretics consider 120 cols to be ok for libcamera, and\n> nowadays it is accepted for linux as well !! )\n>\nThanks! Adopted.\n\n\n>\n> > +{\n> > + for (unsigned i = 0; i < count; ++i) {\n>\n> I wonder if we should be stricter when defining this interface: is\n> there a maximum number of buffers that can be allocated ? How many\n> planes can a buffer have ? (I don't see this addressed in our\n> FrameBuffer class, so it can be left out from here too ?)\n>\n\nHmm, as it's just a helper function, I think the users should know what\nthey're doing, unless we have more specific rules, like the maximum\nnumber of planes in FrameBuffer.\n\nWe can't stop users allocating too many buffers with or without this\nhelper function, right :p ?\n\n\n>\n> > + std::unique_ptr buffer =\n> > + createBuffer(\"frame-\" + std::to_string(i),\n> frameSizes);\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> if you want to have all the requested buffers to be allocated, and do\n> not allow less than that, there's no point in returning count here, you\n> can return 0\n>\n\nHmm, you're right, while I wonder if it's the same case for\nV4L2VideoDevice::exportBuffers/createBuffers? I want to align with\nother exportBuffers API.\n\n\n>\n> > +}\n> > +\n> > +std::unique_ptr DmaBufAllocator::createBuffer(\n> > + std::string name, std::vector frameSizes)\n>\n> Or\n>\n> std::unique_ptr\n> DmaBufAllocator::createBuffer(std::string name,\n> const std::vector &frameSizes)\n>\n> can frameSize be passed as a const reference ?\n>\nThanks! Adopted.\n\n\n>\n>\n> > +{\n> > + std::vector planes;\n> > +\n> > + std::size_t bufferSize = 0, offset = 0;\n> > + for (auto frameSize : frameSizes)\n> > + bufferSize += frameSize;\n>\n> This API allows unbounded size allocation, just sayin', maybe it's\n> fine\n>\n\nYeah... as it's just a helper function, I still think that the users should\nknow what they're doing.\n\n\n>\n> > +\n> > + SharedFD fd(alloc(name.c_str(), bufferSize));\n> > + if (!fd.isValid())\n> > + return nullptr;\n> > +\n> > + for (auto frameSize : frameSizes) {\n> > + FrameBuffer::Plane plane;\n> > + plane.fd = fd;\n> > + plane.offset = offset;\n> > + plane.length = frameSize;\n> > + planes.push_back(std::move(plane));\n> > + offset += plane.length;\n> > + }\n>\n> With a little implicit type casting, you can\n>\n> unsigned int offset = 0;\n> for (unsigned int frameSize : frameSizes) {\n> planes.emplace_back(FrameBuffer::Plane{fd, offset,\n> frameSize});\n> offset += frameSize;\n> }\n>\n> But it won't save you going a temporary object I'm afraid. I'm not\n> sure it's actually any better, up to you.\n>\n> Thanks, adopted. Also, as the offset's and length's types are both\nunsigned int, I changed `frameSize` to be an array of unsigned as\nwell.\n\n\n> > +\n> > + return std::make_unique(planes);\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > --\n> > 2.46.0.184.g6999bdac58-goog\n> >\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 B0B09C323E\n\tfor ;\n\tTue, 27 Aug 2024 15:34:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5AE9D63466;\n\tTue, 27 Aug 2024 17:34:57 +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 A30B361901\n\tfor ;\n\tTue, 27 Aug 2024 17:34:55 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2f029e9c9cfso68584821fa.2\n\tfor ;\n\tTue, 27 Aug 2024 08:34:55 -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=\"BaywG3lf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724772895; x=1725377695;\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=3sqVNLyYpce7qy/lxqC7/0nNbM7XdTW3XySggq5P8fY=;\n\tb=BaywG3lfl70xPOrhVsWo8JQdRahf8WcFZJsJW5Uy1YLUBoOGmz5JLW3VvTsOdn7XhF\n\tDEZGjAWXUNj8CrN7KBZuvsldOMEaTS7Gn4wtJMGU/n1S5+4LDvr1v3RufWns8b5TgqIb\n\taLQKzGqMl20MG1pn+Nx8tM59yEruR+bvWs9FQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724772895; x=1725377695;\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=3sqVNLyYpce7qy/lxqC7/0nNbM7XdTW3XySggq5P8fY=;\n\tb=TKhmIINLc06uGkbuJQ7YnbHduTSXXBJrSybL1mQ0l1OML2QxENZpES6a+NDWdAP6IE\n\tPmE32vJY15wz1MWDT0VqL6zdPvRr95oUIQnDvCS7ZHs+VUTlYkTwNJCvRIxeNO2oRXf7\n\tufpBNVibIbaCTuf/1oARnhMlDH2tbTfGb8lvxWu2pO0+PshL2wvrvfKty52rZeITJywT\n\tYLvqOAzugyxicBJPU89GiBE31EEQUxbwXYHBmB3uvqfWrXwT16WE4pDdcDJ0tv/VXPNF\n\tH2QL75MDyPbVB73LsUr183r9DQZs+uCrh+sdkIW40pHyVSu2fu8X6kCiA38cSwH0iN+N\n\tXxwg==","X-Gm-Message-State":"AOJu0YwIQ+LmtL8nsiwnMBEHh1GPee/xohkDNDYrB/iwhH1A9qXjl/iS\n\tFsfcZPkoFt1bfaHuES6DCQU5l6iQQcOKgGKRSnkhOCZdjb32xZ+nlk19nGNBOO98/XksAIWzeJu\n\tBftgVqoy+K9JRpvDkYFVrbLOTru/5zVDsAOmD","X-Google-Smtp-Source":"AGHT+IFIY3Y29mIMakzDq5SGlP5ZTemvgYWy3Sm6MmA9RefFL/f1HFux0w89IVj74enD/CHJ6JkoVsyJ7MP0GJyTDCU=","X-Received":"by 2002:a2e:91d3:0:b0:2ec:4093:ec7 with SMTP id\n\t38308e7fff4ca-2f4f57923b4mr105371741fa.30.1724772894615;\n\tTue, 27 Aug 2024 08:34:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-2-chenghaoyang@google.com>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Tue, 27 Aug 2024 17:34:43 +0200","Message-ID":"","Subject":"Re: [PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Content-Type":"multipart/alternative; boundary=\"000000000000b3c23c0620abfb25\"","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":31107,"web_url":"https://patchwork.libcamera.org/comment/31107/","msgid":"","date":"2024-09-07T14:33:07","subject":"Re: [PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Sat, Aug 31, 2024 at 8:39 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Tue, Aug 27, 2024 at 05:34:43PM GMT, Cheng-Hao Yang wrote:\n> > Thanks Jacopo for the review.\n> >\n> > The corresponding updates are included in v9.1. Please check.\n> >\n> > On Tue, Aug 27, 2024 at 4:13 PM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > wrote:\n> >\n> > > Hi Harvey\n> > >\n> > > On Tue, Aug 20, 2024 at 04:23:32PM GMT, Harvey Yang wrote:\n> > > > Add a helper function exportBuffers in DmaBufAllocator to make it\n> easier\n> > > > to use.\n> > > >\n> > > > It'll be used in Virtual Pipeline Handler and SoftwareIsp.\n> > > >\n> > > > Signed-off-by: Harvey Yang \n> > > > ---\n> > > > .../libcamera/internal/dma_buf_allocator.h | 12 ++++\n> > > > src/libcamera/dma_buf_allocator.cpp | 64\n> ++++++++++++++++++-\n> > > > 2 files changed, 74 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 36ec1696b..3a9b56b1c 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> > > Where do you use this ?\n> > >\n> > >\n> > Right, it's only used in the previous versions. Removed.\n> >\n> >\n> > > > +\n> > > > class DmaBufAllocator\n> > > > {\n> > > > public:\n> > > > @@ -30,7 +34,15 @@ public:\n> > > > bool isValid() const { return providerHandle_.isValid(); }\n> > > > UniqueFD alloc(const char *name, std::size_t size);\n> > > >\n> > > > + int exportBuffers(\n> > >\n> > > weird indent\n> > >\n> > > int exportBuffers(std::size_t count,\n> > > const std::vector &frameSize,\n> > > std::vector>\n> > > *buffers);\n> > >\n> > > Done\n>\n> Sometimes you replies are indendented at the same level as the\n> previous email you replied to, making it really hard to distinguish\n> the two. Could you check why it happens ?\n>\n>\nAh, thanks for letting me know. I normally wouldn't leave an empty\nline before my replies, and if the reply line above is an empty line,\nthis issue happens.\n\nI'll leave one extra line above every reply from now on.\n\n>\n> >\n> > > > + std::size_t count,\n> > >\n> > > #include \n> > >\n> > Removed std::size_t.\n> >\n> >\n> > >\n> > > > + std::vector frameSize,\n> > > > + std::vector> *buffers);\n> > >\n> > > #include \n> > >\n> > > Done\n> >\n> >\n> > > > +\n> > > > private:\n> > > > + std::unique_ptr createBuffer(\n> > > > + std::string name, std::vector frameSizes);\n> > >\n> > > #include \n> > >\n> > Done\n> >\n> >\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 c06eca7d0..2d88b8b2b 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> > > Not used\n> > >\n> > Removed\n> >\n> >\n> > >\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\n> > >\n> > > Removed the changes\n> >\n> >\n> > > > #endif\n> > > > #endif\n> > > >\n> > > > @@ -243,4 +248,59 @@ UniqueFD DmaBufAllocator::alloc(const char\n> *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> > >\n> > > There's no \\a stream\n> > >\n> > Removed.\n> >\n> >\n> > >\n> > > > + * \\param[in] count The number of FrameBuffers required\n> > >\n> > > \"of requested FrameBuffers\" ?\n> > >\n> > Thanks! Adopted.\n> >\n> >\n> > >\n> > > > + * \\param[in] frameSizes The sizes of planes in the FrameBuffer\n> > > > + * \\param[out] buffers Array of buffers successfully allocated\n> > > > + *\n> > > > + * \\return The number of allocated buffers on success or a negative\n> > > error code\n> > > > + * otherwise\n> > > > + */\n> > > > +int DmaBufAllocator::exportBuffers(\n> > > > + std::size_t count,\n> > >\n> > > nit: This can be an unsigned int maybe\n> > >\n> > Done\n> >\n> >\n> > >\n> > > > + std::vector frameSizes,\n> > >\n> > > can this be passed as a const reference ?\n> > >\n> > Done\n> >\n> >\n> > >\n> > > > + std::vector> *buffers)\n> > >\n> > > weird indentation, I think\n> > >\n> > > int DmaBufAllocator::exportBuffers(std::size_t count,\n> > > const std::vector\n> > > &frameSizes,\n> > >\n> > > std::vector> *buffers)\n> > >\n> > > would do (some heretics consider 120 cols to be ok for libcamera, and\n> > > nowadays it is accepted for linux as well !! )\n> > >\n> > Thanks! Adopted.\n> >\n> >\n> > >\n> > > > +{\n> > > > + for (unsigned i = 0; i < count; ++i) {\n> > >\n> > > I wonder if we should be stricter when defining this interface: is\n> > > there a maximum number of buffers that can be allocated ? How many\n> > > planes can a buffer have ? (I don't see this addressed in our\n> > > FrameBuffer class, so it can be left out from here too ?)\n> > >\n> >\n> > Hmm, as it's just a helper function, I think the users should know what\n> > they're doing, unless we have more specific rules, like the maximum\n> > number of planes in FrameBuffer.\n>\n> Never assume how an API could be wrongly used :)\n>\n> >\n> > We can't stop users allocating too many buffers with or without this\n> > helper function, right :p ?\n> >\n>\n> True indeed, but a good API should minimize the surface for making it\n> used wrongly, even more when it gives access to system resource.\n>\n> See below.\n>\n> >\n> > >\n> > > > + std::unique_ptr buffer =\n> > > > + createBuffer(\"frame-\" + std::to_string(i),\n> > > frameSizes);\n> > > > + if (!buffer) {\n> > > > + LOG(DmaBufAllocator, Error) << \"Unable to\n> 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> > > if you want to have all the requested buffers to be allocated, and do\n> > > not allow less than that, there's no point in returning count here, you\n> > > can return 0\n> > >\n> >\n> > Hmm, you're right, while I wonder if it's the same case for\n> > V4L2VideoDevice::exportBuffers/createBuffers? I want to align with\n> > other exportBuffers API.\n> >\n>\n> I think it's fine\n>\n> >\n> > >\n> > > > +}\n> > > > +\n> > > > +std::unique_ptr DmaBufAllocator::createBuffer(\n> > > > + std::string name, std::vector frameSizes)\n> > >\n> > > Or\n> > >\n> > > std::unique_ptr\n> > > DmaBufAllocator::createBuffer(std::string name,\n> > > const std::vector\n> &frameSizes)\n> > >\n> > > can frameSize be passed as a const reference ?\n> > >\n> > Thanks! Adopted.\n> >\n> >\n> > >\n> > >\n> > > > +{\n> > > > + std::vector planes;\n> > > > +\n> > > > + std::size_t bufferSize = 0, offset = 0;\n> > > > + for (auto frameSize : frameSizes)\n> > > > + bufferSize += frameSize;\n> > >\n> > > This API allows unbounded size allocation, just sayin', maybe it's\n> > > fine\n> > >\n> >\n> > Yeah... as it's just a helper function, I still think that the users\n> should\n> > know what they're doing.\n> >\n>\n> As long as they don't :)\n>\n> I'm particularly concerned about udmabuf, as it uses memfd as backing\n> storage, and we allow to allocate a lof of memory (we did already to\n> be completely honest, as a user can call alloc with any size).\n>\n> Also, I've now noticed the dma_buf_allocator API are not public, so\n> this makes me way less concerned.\n>\n\nYes, pipeline handlers' developers should be more careful than the\napplications'.\n\nSo let's say we keep the API as is for now, until there are more guidelines\non FrameBuffer's limitation?\n\n\n>\n> >\n> > >\n> > > > +\n> > > > + SharedFD fd(alloc(name.c_str(), bufferSize));\n> > > > + if (!fd.isValid())\n> > > > + return nullptr;\n> > > > +\n> > > > + for (auto frameSize : frameSizes) {\n> > > > + FrameBuffer::Plane plane;\n> > > > + plane.fd = fd;\n> > > > + plane.offset = offset;\n> > > > + plane.length = frameSize;\n> > > > + planes.push_back(std::move(plane));\n> > > > + offset += plane.length;\n> > > > + }\n> > >\n> > > With a little implicit type casting, you can\n> > >\n> > > unsigned int offset = 0;\n> > > for (unsigned int frameSize : frameSizes) {\n> > > planes.emplace_back(FrameBuffer::Plane{fd, offset,\n> > > frameSize});\n> > > offset += frameSize;\n> > > }\n> > >\n> > > But it won't save you going a temporary object I'm afraid. I'm not\n> > > sure it's actually any better, up to you.\n> > >\n> > > Thanks, adopted. Also, as the offset's and length's types are both\n> > unsigned int, I changed `frameSize` to be an array of unsigned as\n> > well.\n> >\n> >\n> > > > +\n> > > > + return std::make_unique(planes);\n> > > > +}\n> > > > +\n> > > > } /* namespace libcamera */\n> > > > --\n> > > > 2.46.0.184.g6999bdac58-goog\n> > > >\n> > >\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 BD178C324C\n\tfor ;\n\tSat, 7 Sep 2024 14:33:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 51BF5634E9;\n\tSat, 7 Sep 2024 16:33:21 +0200 (CEST)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92884634E9\n\tfor ;\n\tSat, 7 Sep 2024 16:33:19 +0200 (CEST)","by mail-lj1-x22b.google.com with SMTP id\n\t38308e7fff4ca-2f75c0b78fbso5454441fa.1\n\tfor ;\n\tSat, 07 Sep 2024 07:33:19 -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=\"OGSfXCPD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725719599; x=1726324399;\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=0P+e/EQ6UbQ0tDOzr7ljB2dOplEIrxFFYGmHdX1heC0=;\n\tb=OGSfXCPDfZfl+Ve4GOrhdRrjtg6iCDxDoXqEtluz8yT/WBrktFmOCocZZp5WX1uzBv\n\t67g26Y0C51MDyQtX5pXsWMi9yj54ApRg/5YSMMj/S3pPeHitpZ6F3URIbpFBAo4FtSi6\n\t0NFOAi69vtS170l6Q64p7WpwKJH3WYlSHuyFM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725719599; x=1726324399;\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=0P+e/EQ6UbQ0tDOzr7ljB2dOplEIrxFFYGmHdX1heC0=;\n\tb=RbaV231NYdFtcw1tiMoW6wSOLtrg5SxJarFHRYw4HFLkJ/AjQ2rg3XKMMFrMvHZ2JP\n\tE37YHdjcstMhQ6t/B0f98YegVcB5a13BDr0q6GAKZ0mg82K4HDrUO5PEkr00GT9iQebT\n\tPQN2sWcMx7E6PtcdKsZV9BgvSJdjovcSUfLJBOCPvvo6UhxVl4I7LfVOxio0c1FAfPfx\n\t32rIKEfxiPKmrj9J2OH7N8M0svY/NxG8CCCCD9DLGEUICAsr994hv1toXdvNeyVpcPWx\n\t7mFV7nus5nd0MSBZgCythloUTOqmjh+YYRB48xXCFKDQLiSJbukGf+JzrD7lzHkWIgnP\n\tqB+w==","X-Gm-Message-State":"AOJu0YyW96xLIksJsV0TAFeVWjzfl19lS9PVxAnEPwA6mTxUdAG1BFSU\n\tngIHCrbsRGbk+c3doaRopilF0s2WuJkWxq9nazARps35Gz48SyBrCAP4gOPW9ltPDK24lUdiCBE\n\tyY0ui6ZrzcMKE/3JVIvS11t6+GJGSnssmVmaKQr2wxK+vq74=","X-Google-Smtp-Source":"AGHT+IF/M4xABIhwibGaPgA8jSZRzR9iIappCndCLD0K4pY/PUUQKdq4Kutyk4d5Im1i74JCtwZKZ20igNvpHbUBP/s=","X-Received":"by 2002:a2e:744:0:b0:2f6:6d6d:63d1 with SMTP id\n\t38308e7fff4ca-2f751f9a8demr36338171fa.44.1725719598422;\n\tSat, 07 Sep 2024 07:33:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-2-chenghaoyang@google.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Sat, 7 Sep 2024 22:33:07 +0800","Message-ID":"","Subject":"Re: [PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Content-Type":"multipart/alternative; boundary=\"000000000000a57a1406218867a2\"","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\" "}}]