[{"id":18649,"web_url":"https://patchwork.libcamera.org/comment/18649/","msgid":"","date":"2021-08-10T02:05:45","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: vimc: Map and unmap buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Aug 06, 2021 at 03:44:08PM +0530, Umang Jain wrote:\n> Since VIMC is a virtual test driver, the support for IPA\n> buffers(parameter and statistics) is missing. We could create\n\ns/buffers/buffers /\n\n> our own and pass it around, but created buffers needs to be\n> backed by dmabuf file-descriptors ideally.\n> \n> This might prove cubersome given that we use vimc for test purposes.\n> Hence, pass actual frame data buffers into the IPA instead, which\n> would result in exercising these code paths for now. Introduce plumbing\n> support for mapping and un-mapping of these buffers into the IPA.\n\nI know what this is about, but if I didn't I'd have trouble following\nthe rationale. Here's a possible improvement.\n\n\nVIMC is a virtual test driver that doesn't generate statistics or\nconsume parameters buffers. Its pipeline handler thus doesn't map any\nbuffer to the IPA. This limits the ability to test corresponding code\npaths in unit tests, as they use VIMC extensively.\n\nCreating fake statistics and parameters buffers would be cumbersome. Map\nthe frame buffers to the IPA instead, to extend test coverage.\n\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/ipa/vimc.mojom | 3 +++\n> src/ipa/vimc/vimc.cpp | 26 ++++++++++++++++++++++++++\n> src/libcamera/pipeline/vimc/vimc.cpp | 25 ++++++++++++++++++++++++-\n> 3 files changed, 53 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n> index 99b6412b..8452461d 100644\n> --- a/include/libcamera/ipa/vimc.mojom\n> +++ b/include/libcamera/ipa/vimc.mojom\n> @@ -26,6 +26,9 @@ interface IPAVimcInterface {\n> \n> \tstart() => (int32 ret);\n> \tstop();\n> +\n> +\tmapBuffers(array buffers);\n> +\tunmapBuffers(array ids);\n> };\n> \n> interface IPAVimcEventInterface {\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index 54d9086a..2b00687f 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -19,6 +19,8 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/framebuffer.h\"\n> +\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPAVimc)\n> @@ -38,11 +40,15 @@ public:\n> \t\t const std::map &streamConfig,\n> \t\t const std::map &entityControls) override;\n> \n> +\tvoid mapBuffers(const std::vector &buffers) override;\n> +\tvoid unmapBuffers(const std::vector &ids) override;\n> +\n> private:\n> \tvoid initTrace();\n> \tvoid trace(enum ipa::vimc::IPATraceCode operation);\n> \n> \tint fd_;\n> +\tstd::map buffers_;\n> };\n> \n> IPAVimc::IPAVimc()\n> @@ -99,6 +105,26 @@ int IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo,\n> \treturn 0;\n> }\n> \n> +void IPAVimc::mapBuffers(const std::vector &buffers)\n> +{\n> +\tfor (const IPABuffer &buffer : buffers) {\n> +\t\tconst FrameBuffer fb(buffer.planes);\n> +\t\tbuffers_.emplace(buffer.id,\n> +\t\t\t\t MappedFrameBuffer(&fb, PROT_READ));\n> +\t}\n> +}\n> +\n> +void IPAVimc::unmapBuffers(const std::vector &ids)\n> +{\n> +\tfor (unsigned int id : ids) {\n> +\t\tauto it = buffers_.find(id);\n> +\t\tif (it == buffers_.end())\n> +\t\t\tcontinue;\n> +\n> +\t\tbuffers_.erase(it);\n> +\t}\n> +}\n> +\n> void IPAVimc::initTrace()\n> {\n> \tstruct stat fifoStat;\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index b7dd6595..fa21128d 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -45,7 +45,7 @@ class VimcCameraData : public CameraData\n> {\n> public:\n> \tVimcCameraData(PipelineHandler *pipe, MediaDevice *media)\n> -\t\t: CameraData(pipe), media_(media)\n> +\t\t: CameraData(pipe), media_(media), buffers_(nullptr)\n> \t{\n> \t}\n> \n> @@ -61,6 +61,9 @@ public:\n> \tStream stream_;\n> \n> \tstd::unique_ptr ipa_;\n> +\n> +\tstd::vector> *buffers_;\n> +\tstd::vector ipaBuffers_;\n> };\n> \n> class VimcCameraConfiguration : public CameraConfiguration\n> @@ -319,6 +322,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> \tunsigned int count = stream->configuration().bufferCount;\n> +\tdata->buffers_ = buffers;\n> \n> \treturn data->video_->exportBuffers(count, buffers);\n> }\n> @@ -332,6 +336,19 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> +\t/*\n> +\t * We don't have actual parameters/statistics buffers in VIMC.\n> +\t * Hence, map frame buffers to exercise IPA IPC code paths for now.\n> +\t */\n> +\tASSERT(data->buffers_ != nullptr);\n\nWe have a problem here. There's nothing that guarantees that the frame\nbuffers that have been queued have been exported by the pipeline\nhandler. I'm afraid we won't be able to use frame buffers for this :-(\nIt looks like we'll have to instead allocate fake stats buffers, even if\nit's cumbersome. There are at least two options to create dmabufs from\nuserspace, dmabuf heaps or udmabuf. I'm not sure which one is best, but\nlet's take into account the default permissions that distributions set\non the related devices, to minimize issues.\n\n> +\tunsigned int ipaBufferId = 1;\n> +\tfor (unsigned int i = 0; i< count; i++) {\n> +\t\tstd::unique_ptr &buffer = data->buffers_->at(i);\n> +\t\tbuffer->setCookie(ipaBufferId++);\n> + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n> +\t}\n> +\tdata->ipa_->mapBuffers(data->ipaBuffers_);\n> +\n> \tret = data->ipa_->start();\n> \tif (ret) {\n> \t\tdata->video_->releaseBuffers();\n> @@ -352,7 +369,13 @@ void PipelineHandlerVimc::stop(Camera *camera)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> \tdata->video_->streamOff();\n> +\n> +\tstd::vector ids;\n> +\tfor (IPABuffer &ipabuf : data->ipaBuffers_)\n> +\t\tids.push_back(ipabuf.id);\n> +\tdata->ipa_->unmapBuffers(ids);\n> \tdata->ipa_->stop();\n> +\n> \tdata->video_->releaseBuffers();\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 A10E0BD87D\n\tfor ;\n\tTue, 10 Aug 2021 02:05:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0147D6884F;\n\tTue, 10 Aug 2021 04:05:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 451CC687EB\n\tfor ;\n\tTue, 10 Aug 2021 04:05:48 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B992B3F0;\n\tTue, 10 Aug 2021 04:05:47 +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=\"maeLNLSN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628561147;\n\tbh=uEvQ78nCn+5LbBuAB0o92oG3tCH8yfNSJmNY7dCJba8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=maeLNLSNPoRXqwlb4boWrdGYlNW96L27FUnAIi20+DAbBdnjogKbROWBpaTBirJXD\n\tMac6P0l+pMZT034xGeJq7jMTe6Iu/TtyWpdwE7Q6mt4Ojo8s0drp6Z4svqyCZEuD/o\n\tl4aUbgkC3dsOiPfv/UmNYCAYk+GAKGJZDQqQmVSw=","Date":"Tue, 10 Aug 2021 05:05:45 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210806101409.324645-1-umang.jain@ideasonboard.com>\n\t<20210806101409.324645-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210806101409.324645-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: vimc: Map and unmap buffers","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18654,"web_url":"https://patchwork.libcamera.org/comment/18654/","msgid":"","date":"2021-08-10T03:22:59","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: vimc: Map and unmap buffers","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/10/21 7:35 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Aug 06, 2021 at 03:44:08PM +0530, Umang Jain wrote:\n>> Since VIMC is a virtual test driver, the support for IPA\n>> buffers(parameter and statistics) is missing. We could create\n> s/buffers/buffers /\n>\n>> our own and pass it around, but created buffers needs to be\n>> backed by dmabuf file-descriptors ideally.\n>>\n>> This might prove cubersome given that we use vimc for test purposes.\n>> Hence, pass actual frame data buffers into the IPA instead, which\n>> would result in exercising these code paths for now. Introduce plumbing\n>> support for mapping and un-mapping of these buffers into the IPA.\n> I know what this is about, but if I didn't I'd have trouble following\n> the rationale. Here's a possible improvement.\n>\n>\n> VIMC is a virtual test driver that doesn't generate statistics or\n> consume parameters buffers. Its pipeline handler thus doesn't map any\n> buffer to the IPA. This limits the ability to test corresponding code\n> paths in unit tests, as they use VIMC extensively.\n>\n> Creating fake statistics and parameters buffers would be cumbersome. Map\n> the frame buffers to the IPA instead, to extend test coverage.\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/ipa/vimc.mojom | 3 +++\n>> src/ipa/vimc/vimc.cpp | 26 ++++++++++++++++++++++++++\n>> src/libcamera/pipeline/vimc/vimc.cpp | 25 ++++++++++++++++++++++++-\n>> 3 files changed, 53 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n>> index 99b6412b..8452461d 100644\n>> --- a/include/libcamera/ipa/vimc.mojom\n>> +++ b/include/libcamera/ipa/vimc.mojom\n>> @@ -26,6 +26,9 @@ interface IPAVimcInterface {\n>> \n>> \tstart() => (int32 ret);\n>> \tstop();\n>> +\n>> +\tmapBuffers(array buffers);\n>> +\tunmapBuffers(array ids);\n>> };\n>> \n>> interface IPAVimcEventInterface {\n>> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n>> index 54d9086a..2b00687f 100644\n>> --- a/src/ipa/vimc/vimc.cpp\n>> +++ b/src/ipa/vimc/vimc.cpp\n>> @@ -19,6 +19,8 @@\n>> #include \n>> #include \n>> \n>> +#include \"libcamera/internal/framebuffer.h\"\n>> +\n>> namespace libcamera {\n>> \n>> LOG_DEFINE_CATEGORY(IPAVimc)\n>> @@ -38,11 +40,15 @@ public:\n>> \t\t const std::map &streamConfig,\n>> \t\t const std::map &entityControls) override;\n>> \n>> +\tvoid mapBuffers(const std::vector &buffers) override;\n>> +\tvoid unmapBuffers(const std::vector &ids) override;\n>> +\n>> private:\n>> \tvoid initTrace();\n>> \tvoid trace(enum ipa::vimc::IPATraceCode operation);\n>> \n>> \tint fd_;\n>> +\tstd::map buffers_;\n>> };\n>> \n>> IPAVimc::IPAVimc()\n>> @@ -99,6 +105,26 @@ int IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo,\n>> \treturn 0;\n>> }\n>> \n>> +void IPAVimc::mapBuffers(const std::vector &buffers)\n>> +{\n>> +\tfor (const IPABuffer &buffer : buffers) {\n>> +\t\tconst FrameBuffer fb(buffer.planes);\n>> +\t\tbuffers_.emplace(buffer.id,\n>> +\t\t\t\t MappedFrameBuffer(&fb, PROT_READ));\n>> +\t}\n>> +}\n>> +\n>> +void IPAVimc::unmapBuffers(const std::vector &ids)\n>> +{\n>> +\tfor (unsigned int id : ids) {\n>> +\t\tauto it = buffers_.find(id);\n>> +\t\tif (it == buffers_.end())\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tbuffers_.erase(it);\n>> +\t}\n>> +}\n>> +\n>> void IPAVimc::initTrace()\n>> {\n>> \tstruct stat fifoStat;\n>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n>> index b7dd6595..fa21128d 100644\n>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n>> @@ -45,7 +45,7 @@ class VimcCameraData : public CameraData\n>> {\n>> public:\n>> \tVimcCameraData(PipelineHandler *pipe, MediaDevice *media)\n>> -\t\t: CameraData(pipe), media_(media)\n>> +\t\t: CameraData(pipe), media_(media), buffers_(nullptr)\n>> \t{\n>> \t}\n>> \n>> @@ -61,6 +61,9 @@ public:\n>> \tStream stream_;\n>> \n>> \tstd::unique_ptr ipa_;\n>> +\n>> +\tstd::vector> *buffers_;\n>> +\tstd::vector ipaBuffers_;\n>> };\n>> \n>> class VimcCameraConfiguration : public CameraConfiguration\n>> @@ -319,6 +322,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,\n>> {\n>> \tVimcCameraData *data = cameraData(camera);\n>> \tunsigned int count = stream->configuration().bufferCount;\n>> +\tdata->buffers_ = buffers;\n>> \n>> \treturn data->video_->exportBuffers(count, buffers);\n>> }\n>> @@ -332,6 +336,19 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>> \n>> +\t/*\n>> +\t * We don't have actual parameters/statistics buffers in VIMC.\n>> +\t * Hence, map frame buffers to exercise IPA IPC code paths for now.\n>> +\t */\n>> +\tASSERT(data->buffers_ != nullptr);\n> We have a problem here. There's nothing that guarantees that the frame\n> buffers that have been queued have been exported by the pipeline\n> handler. I'm afraid we won't be able to use frame buffers for this :-(\n\nI don't understand. We are using buffers which are exported (see the \nabove hunk of exportFormatBuffers())\n\nThe guarantees you have mentioned, Is it about checking\ndata->video_->exportBuffers(count, buffers)\n\nhas been successful, before using the buffers for IPA paths?\n\n> It looks like we'll have to instead allocate fake stats buffers, even if\n> it's cumbersome. There are at least two options to create dmabufs from\n> userspace, dmabuf heaps or udmabuf. I'm not sure which one is best, but\n> let's take into account the default permissions that distributions set\n> on the related devices, to minimize issues.\n>\n>> +\tunsigned int ipaBufferId = 1;\n>> +\tfor (unsigned int i = 0; i< count; i++) {\n>> +\t\tstd::unique_ptr &buffer = data->buffers_->at(i);\n>> +\t\tbuffer->setCookie(ipaBufferId++);\n>> + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n>> +\t}\n>> +\tdata->ipa_->mapBuffers(data->ipaBuffers_);\n>> +\n>> \tret = data->ipa_->start();\n>> \tif (ret) {\n>> \t\tdata->video_->releaseBuffers();\n>> @@ -352,7 +369,13 @@ void PipelineHandlerVimc::stop(Camera *camera)\n>> {\n>> \tVimcCameraData *data = cameraData(camera);\n>> \tdata->video_->streamOff();\n>> +\n>> +\tstd::vector ids;\n>> +\tfor (IPABuffer &ipabuf : data->ipaBuffers_)\n>> +\t\tids.push_back(ipabuf.id);\n>> +\tdata->ipa_->unmapBuffers(ids);\n>> \tdata->ipa_->stop();\n>> +\n>> \tdata->video_->releaseBuffers();\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 2DDD1BD87D\n\tfor ;\n\tTue, 10 Aug 2021 03:23:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A91D68826;\n\tTue, 10 Aug 2021 05:23:05 +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 4B8A760262\n\tfor ;\n\tTue, 10 Aug 2021 05:23:04 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 157B43F0;\n\tTue, 10 Aug 2021 05:23:02 +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=\"wnkzUFrU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628565783;\n\tbh=ELr7KukrDmrAY2EbTPXUQCqdkCyZSNKA1VVXU1t10XY=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=wnkzUFrU5m/mEahjEtYqCwtQoYjuuIApZ1djI20jJQ8mR4OP89Jq/VBJ+6aNqzU/R\n\tPyPzIlgSEiMdibzXER86Xu+/exbEuOb0AaALP4J8fU03/RvRJ4jqtTENtLfjjlYgiN\n\tik6QD9Th3QGR41aPCF1m9WG64W2eoj47TVpyWTbg=","To":"Laurent Pinchart ","References":"<20210806101409.324645-1-umang.jain@ideasonboard.com>\n\t<20210806101409.324645-4-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Tue, 10 Aug 2021 08:52:59 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: vimc: Map and unmap buffers","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18670,"web_url":"https://patchwork.libcamera.org/comment/18670/","msgid":"","date":"2021-08-10T10:18:02","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: vimc: Map and unmap buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Aug 10, 2021 at 08:52:59AM +0530, Umang Jain wrote:\n> On 8/10/21 7:35 AM, Laurent Pinchart wrote:\n> > On Fri, Aug 06, 2021 at 03:44:08PM +0530, Umang Jain wrote:\n> >> Since VIMC is a virtual test driver, the support for IPA\n> >> buffers(parameter and statistics) is missing. We could create\n> > s/buffers/buffers /\n> >\n> >> our own and pass it around, but created buffers needs to be\n> >> backed by dmabuf file-descriptors ideally.\n> >>\n> >> This might prove cubersome given that we use vimc for test purposes.\n> >> Hence, pass actual frame data buffers into the IPA instead, which\n> >> would result in exercising these code paths for now. Introduce plumbing\n> >> support for mapping and un-mapping of these buffers into the IPA.\n> >\n> > I know what this is about, but if I didn't I'd have trouble following\n> > the rationale. Here's a possible improvement.\n> >\n> > VIMC is a virtual test driver that doesn't generate statistics or\n> > consume parameters buffers. Its pipeline handler thus doesn't map any\n> > buffer to the IPA. This limits the ability to test corresponding code\n> > paths in unit tests, as they use VIMC extensively.\n> >\n> > Creating fake statistics and parameters buffers would be cumbersome. Map\n> > the frame buffers to the IPA instead, to extend test coverage.\n> >\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> include/libcamera/ipa/vimc.mojom | 3 +++\n> >> src/ipa/vimc/vimc.cpp | 26 ++++++++++++++++++++++++++\n> >> src/libcamera/pipeline/vimc/vimc.cpp | 25 ++++++++++++++++++++++++-\n> >> 3 files changed, 53 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n> >> index 99b6412b..8452461d 100644\n> >> --- a/include/libcamera/ipa/vimc.mojom\n> >> +++ b/include/libcamera/ipa/vimc.mojom\n> >> @@ -26,6 +26,9 @@ interface IPAVimcInterface {\n> >> \n> >> \tstart() => (int32 ret);\n> >> \tstop();\n> >> +\n> >> +\tmapBuffers(array buffers);\n> >> +\tunmapBuffers(array ids);\n> >> };\n> >> \n> >> interface IPAVimcEventInterface {\n> >> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> >> index 54d9086a..2b00687f 100644\n> >> --- a/src/ipa/vimc/vimc.cpp\n> >> +++ b/src/ipa/vimc/vimc.cpp\n> >> @@ -19,6 +19,8 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \"libcamera/internal/framebuffer.h\"\n> >> +\n> >> namespace libcamera {\n> >> \n> >> LOG_DEFINE_CATEGORY(IPAVimc)\n> >> @@ -38,11 +40,15 @@ public:\n> >> \t\t const std::map &streamConfig,\n> >> \t\t const std::map &entityControls) override;\n> >> \n> >> +\tvoid mapBuffers(const std::vector &buffers) override;\n> >> +\tvoid unmapBuffers(const std::vector &ids) override;\n> >> +\n> >> private:\n> >> \tvoid initTrace();\n> >> \tvoid trace(enum ipa::vimc::IPATraceCode operation);\n> >> \n> >> \tint fd_;\n> >> +\tstd::map buffers_;\n> >> };\n> >> \n> >> IPAVimc::IPAVimc()\n> >> @@ -99,6 +105,26 @@ int IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo,\n> >> \treturn 0;\n> >> }\n> >> \n> >> +void IPAVimc::mapBuffers(const std::vector &buffers)\n> >> +{\n> >> +\tfor (const IPABuffer &buffer : buffers) {\n> >> +\t\tconst FrameBuffer fb(buffer.planes);\n> >> +\t\tbuffers_.emplace(buffer.id,\n> >> +\t\t\t\t MappedFrameBuffer(&fb, PROT_READ));\n> >> +\t}\n> >> +}\n> >> +\n> >> +void IPAVimc::unmapBuffers(const std::vector &ids)\n> >> +{\n> >> +\tfor (unsigned int id : ids) {\n> >> +\t\tauto it = buffers_.find(id);\n> >> +\t\tif (it == buffers_.end())\n> >> +\t\t\tcontinue;\n> >> +\n> >> +\t\tbuffers_.erase(it);\n> >> +\t}\n> >> +}\n> >> +\n> >> void IPAVimc::initTrace()\n> >> {\n> >> \tstruct stat fifoStat;\n> >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> >> index b7dd6595..fa21128d 100644\n> >> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> >> @@ -45,7 +45,7 @@ class VimcCameraData : public CameraData\n> >> {\n> >> public:\n> >> \tVimcCameraData(PipelineHandler *pipe, MediaDevice *media)\n> >> -\t\t: CameraData(pipe), media_(media)\n> >> +\t\t: CameraData(pipe), media_(media), buffers_(nullptr)\n> >> \t{\n> >> \t}\n> >> \n> >> @@ -61,6 +61,9 @@ public:\n> >> \tStream stream_;\n> >> \n> >> \tstd::unique_ptr ipa_;\n> >> +\n> >> +\tstd::vector> *buffers_;\n> >> +\tstd::vector ipaBuffers_;\n> >> };\n> >> \n> >> class VimcCameraConfiguration : public CameraConfiguration\n> >> @@ -319,6 +322,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,\n> >> {\n> >> \tVimcCameraData *data = cameraData(camera);\n> >> \tunsigned int count = stream->configuration().bufferCount;\n> >> +\tdata->buffers_ = buffers;\n> >> \n> >> \treturn data->video_->exportBuffers(count, buffers);\n> >> }\n> >> @@ -332,6 +336,19 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> >> \tif (ret < 0)\n> >> \t\treturn ret;\n> >> \n> >> +\t/*\n> >> +\t * We don't have actual parameters/statistics buffers in VIMC.\n> >> +\t * Hence, map frame buffers to exercise IPA IPC code paths for now.\n> >> +\t */\n> >> +\tASSERT(data->buffers_ != nullptr);\n> >\n> > We have a problem here. There's nothing that guarantees that the frame\n> > buffers that have been queued have been exported by the pipeline\n> > handler. I'm afraid we won't be able to use frame buffers for this :-(\n> \n> I don't understand. We are using buffers which are exported (see the \n> above hunk of exportFormatBuffers())\n> \n> The guarantees you have mentioned, Is it about checking\n> data->video_->exportBuffers(count, buffers)\n> \n> has been successful, before using the buffers for IPA paths?\n\nAll our unit tests and test applications use the FrameBufferAllocator,\nbut that's not a requirement. An application may construct FrameBuffer\ninstances itself, using dmabufs obtained elsewhere (exported by a\ndisplay driver for instance).\n\n> > It looks like we'll have to instead allocate fake stats buffers, even if\n> > it's cumbersome. There are at least two options to create dmabufs from\n> > userspace, dmabuf heaps or udmabuf. I'm not sure which one is best, but\n> > let's take into account the default permissions that distributions set\n> > on the related devices, to minimize issues.\n> >\n> >> +\tunsigned int ipaBufferId = 1;\n> >> +\tfor (unsigned int i = 0; i< count; i++) {\n> >> +\t\tstd::unique_ptr &buffer = data->buffers_->at(i);\n> >> +\t\tbuffer->setCookie(ipaBufferId++);\n> >> + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n> >> +\t}\n> >> +\tdata->ipa_->mapBuffers(data->ipaBuffers_);\n> >> +\n> >> \tret = data->ipa_->start();\n> >> \tif (ret) {\n> >> \t\tdata->video_->releaseBuffers();\n> >> @@ -352,7 +369,13 @@ void PipelineHandlerVimc::stop(Camera *camera)\n> >> {\n> >> \tVimcCameraData *data = cameraData(camera);\n> >> \tdata->video_->streamOff();\n> >> +\n> >> +\tstd::vector ids;\n> >> +\tfor (IPABuffer &ipabuf : data->ipaBuffers_)\n> >> +\t\tids.push_back(ipabuf.id);\n> >> +\tdata->ipa_->unmapBuffers(ids);\n> >> \tdata->ipa_->stop();\n> >> +\n> >> \tdata->video_->releaseBuffers();\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 997ADBD87D\n\tfor ;\n\tTue, 10 Aug 2021 10:18:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 617796884D;\n\tTue, 10 Aug 2021 12:18:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8AC43687F0\n\tfor ;\n\tTue, 10 Aug 2021 12:18:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F62F3F0;\n\tTue, 10 Aug 2021 12:18:04 +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=\"LSypkz0A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628590684;\n\tbh=WuzumXgghq9k0VH5FkfCufyTo8kTaIkZYjA+RBOiKiI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LSypkz0Av88zTUI+VWyR7Fsgg9UCeb7HHg/IhhjNliSBMySCkqOfD16quJvt9T51P\n\tjJHTDXQCzsiPHa5ir4nmZPfZ7ybgh52bOl4IprNppeKDaC1+MYCSHkzsVeZCcIw07I\n\tZigKw6YMRAZjIpYZhouv5umhWKKTop4hoCVPAiGw=","Date":"Tue, 10 Aug 2021 13:18:02 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210806101409.324645-1-umang.jain@ideasonboard.com>\n\t<20210806101409.324645-4-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: vimc: Map and unmap buffers","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]