[{"id":18789,"web_url":"https://patchwork.libcamera.org/comment/18789/","msgid":"","date":"2021-08-14T00:35:35","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: vimc: Map and unmap\n\tbuffers","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 13, 2021 at 08:14:36PM +0530, Umang Jain wrote:\n> VIMC pipeline-handler have dmabuf-backed mock FrameBuffers which are\n\ns/pipeline-handler/pipeline handler/\n\n> specifically targetted mimicking IPA buffers(parameter and statistics).\n\ns/buffers/buffers /\n\n> Map these mock buffers to the VIMC IPA that would enable exercising IPA\n> IPC code paths. This will provide leverage to our test suite to test\n> IPA IPC code paths, which are common to various platforms.\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 | 16 ++++++++++++++++\n> 3 files changed, 45 insertions(+)\n> \n> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n> index ee66353d..8cb240d3 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 0535c740..082b2db7 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/mapped_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::IPAOperationCode 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, MappedFrameBuffer::MapFlag::Read));\n\nThe point of emplace compared to insert is to avoid a copy construction.\nThis should be written as\n\n\t\tbuffers_.emplace(std::piecewise_construct,\n\t\t\t\t std::forward_as_tuple(buffer.id),\n\t\t\t\t std::forward_as_tuple(&fb, MappedFrameBuffer::MapFlag::Read));\n\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 edf8c58e..c44a6242 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -63,6 +63,7 @@ public:\n> \n> \tstd::unique_ptr ipa_;\n> \tstd::vector> mockIPABufs_;\n> +\tstd::vector ipaBuffers_;\n> };\n> \n> class VimcCameraConfiguration : public CameraConfiguration\n> @@ -334,6 +335,15 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> +\t/* Map the mock IPA buffers to VIMC IPA to exercise IPC code paths */\n> +\tunsigned int ipaBufferId = 1;\n> +\tfor (unsigned int i = 0; i < data->mockIPABufs_.size(); i++) {\n> +\t\tstd::unique_ptr &buffer = data->mockIPABufs_[i];\n> +\t\tbuffer->setCookie(ipaBufferId++);\n\nYou can use utils::enumerate() here:\n\n\tfor (auto [i, buffer] : utils::enumerate(data->mackIPABufs_)) {\n\t\tbuffer->setCookie(i++);\n\n> +\t\tdata->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n> +\t}\n> +\tdata->ipa_->mapBuffers(data->ipaBuffers_);\n\nThe return value should be checked.\n\n> +\n> \tret = data->ipa_->start();\n> \tif (ret) {\n> \t\tdata->video_->releaseBuffers();\n> @@ -354,7 +364,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\nYou could use here\n\n\tfor (const std::unique_ptr &buffer : data->mockIPABufs_)\n\t\tids.push_back(buffer->cookie());\n\nand replace VimcCameraData::ipaBuffers_ with a local variable in\nPipelineHandlerVimc::start().\n\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 71876BD87D\n\tfor ;\n\tSat, 14 Aug 2021 00:35:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4D1668891;\n\tSat, 14 Aug 2021 02:35:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A96A68823\n\tfor ;\n\tSat, 14 Aug 2021 02:35:40 +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 D82243F0;\n\tSat, 14 Aug 2021 02:35:39 +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=\"YOkgsqXk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628901340;\n\tbh=cr/k19mqpBsNiyuyMqJksQy729KSGus4sfc3q61WBbY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YOkgsqXkm/BcpCBos9n4lTbwZVFhaEGp/UgajxLtAoscGZCwnPXi5ojhrtDWBiQMH\n\tc8zqYBW6+nryfKUgX+2x/cyScxfqe1eStH1hUFPiZbaBPXXFBSaSrwskHqJjFLhV/A\n\t52PPKScJSqWfbInCz03qo3W3VM8HrYkymogYnRF4=","Date":"Sat, 14 Aug 2021 03:35:35 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210813144437.138005-1-umang.jain@ideasonboard.com>\n\t<20210813144437.138005-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210813144437.138005-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: vimc: Map and unmap\n\tbuffers","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":18791,"web_url":"https://patchwork.libcamera.org/comment/18791/","msgid":"<35205819-d96e-5fec-f7da-a135bf5c9856@ideasonboard.com>","date":"2021-08-14T04:56:45","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: vimc: Map and unmap\n\tbuffers","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThanks for review,\n\none comment below\n\nOn 8/14/21 6:05 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Aug 13, 2021 at 08:14:36PM +0530, Umang Jain wrote:\n>> VIMC pipeline-handler have dmabuf-backed mock FrameBuffers which are\n> s/pipeline-handler/pipeline handler/\n>\n>> specifically targetted mimicking IPA buffers(parameter and statistics).\n> s/buffers/buffers /\n>\n>> Map these mock buffers to the VIMC IPA that would enable exercising IPA\n>> IPC code paths. This will provide leverage to our test suite to test\n>> IPA IPC code paths, which are common to various platforms.\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 | 16 ++++++++++++++++\n>> 3 files changed, 45 insertions(+)\n>>\n>> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n>> index ee66353d..8cb240d3 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 0535c740..082b2db7 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/mapped_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::IPAOperationCode 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, MappedFrameBuffer::MapFlag::Read));\n> The point of emplace compared to insert is to avoid a copy construction.\n> This should be written as\n>\n> \t\tbuffers_.emplace(std::piecewise_construct,\n> \t\t\t\t std::forward_as_tuple(buffer.id),\n> \t\t\t\t std::forward_as_tuple(&fb, MappedFrameBuffer::MapFlag::Read));\n>\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 edf8c58e..c44a6242 100644\n>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n>> @@ -63,6 +63,7 @@ public:\n>> \n>> \tstd::unique_ptr ipa_;\n>> \tstd::vector> mockIPABufs_;\n>> +\tstd::vector ipaBuffers_;\n>> };\n>> \n>> class VimcCameraConfiguration : public CameraConfiguration\n>> @@ -334,6 +335,15 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>> \n>> +\t/* Map the mock IPA buffers to VIMC IPA to exercise IPC code paths */\n>> +\tunsigned int ipaBufferId = 1;\n>> +\tfor (unsigned int i = 0; i < data->mockIPABufs_.size(); i++) {\n>> +\t\tstd::unique_ptr &buffer = data->mockIPABufs_[i];\n>> +\t\tbuffer->setCookie(ipaBufferId++);\n> You can use utils::enumerate() here:\n>\n> \tfor (auto [i, buffer] : utils::enumerate(data->mackIPABufs_)) {\n> \t\tbuffer->setCookie(i++);\n>\n>> +\t\tdata->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n>> +\t}\n>> +\tdata->ipa_->mapBuffers(data->ipaBuffers_);\n> The return value should be checked.\n\nI didn't find mapBuffers/unmapBuffers() to return a value, in any of the \nmojom interfaces we have today.\n\nI think mapBuffers() should have return value indeed, can I skip it here \nfor now, and create a patch on top which touches all the concerned IPAs \nas well?\n\n>\n>> +\n>> \tret = data->ipa_->start();\n>> \tif (ret) {\n>> \t\tdata->video_->releaseBuffers();\n>> @@ -354,7 +364,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> You could use here\n>\n> \tfor (const std::unique_ptr &buffer : data->mockIPABufs_)\n> \t\tids.push_back(buffer->cookie());\n>\n> and replace VimcCameraData::ipaBuffers_ with a local variable in\n> PipelineHandlerVimc::start().\n>\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 A95C9C3240\n\tfor ;\n\tSat, 14 Aug 2021 04:56:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F24616888F;\n\tSat, 14 Aug 2021 06:56:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9EA106025E\n\tfor ;\n\tSat, 14 Aug 2021 06:56:51 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.70])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A65403E5;\n\tSat, 14 Aug 2021 06:56:50 +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=\"DBsaA1/K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628917011;\n\tbh=J/XQ4cIwxNnVKquafO8EP6GRXYMaOXIoLOdhpJrOA2c=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=DBsaA1/KTgpMT+LvcsHyRpk+pHJMBaJF3bElhZxNn/ek+iX2coMJ52Ty+nSfXKv0V\n\t6k3ny31WRl2tLE+5Dr9Roq0xDFaOmOR+t7N7idG6fDJ3YOrZeS+CfROwgKPO2nRH+s\n\tKZW4CxFhvVfVCpI4xr/UGNHJr6peimoyiYdjLgW0=","To":"Laurent Pinchart ","References":"<20210813144437.138005-1-umang.jain@ideasonboard.com>\n\t<20210813144437.138005-4-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<35205819-d96e-5fec-f7da-a135bf5c9856@ideasonboard.com>","Date":"Sat, 14 Aug 2021 10:26:45 +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 v2 3/4] ipa: vimc: Map and unmap\n\tbuffers","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":18800,"web_url":"https://patchwork.libcamera.org/comment/18800/","msgid":"","date":"2021-08-14T19:51:43","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: vimc: Map and unmap\n\tbuffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Sat, Aug 14, 2021 at 10:26:45AM +0530, Umang Jain wrote:\n> On 8/14/21 6:05 AM, Laurent Pinchart wrote:\n> > On Fri, Aug 13, 2021 at 08:14:36PM +0530, Umang Jain wrote:\n> >> VIMC pipeline-handler have dmabuf-backed mock FrameBuffers which are\n> >\n> > s/pipeline-handler/pipeline handler/\n> >\n> >> specifically targetted mimicking IPA buffers(parameter and statistics).\n> >\n> > s/buffers/buffers /\n> >\n> >> Map these mock buffers to the VIMC IPA that would enable exercising IPA\n> >> IPC code paths. This will provide leverage to our test suite to test\n> >> IPA IPC code paths, which are common to various platforms.\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 | 16 ++++++++++++++++\n> >> 3 files changed, 45 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n> >> index ee66353d..8cb240d3 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 0535c740..082b2db7 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/mapped_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::IPAOperationCode 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, MappedFrameBuffer::MapFlag::Read));\n> >\n> > The point of emplace compared to insert is to avoid a copy construction.\n> > This should be written as\n> >\n> > \t\tbuffers_.emplace(std::piecewise_construct,\n> > \t\t\t\t std::forward_as_tuple(buffer.id),\n> > \t\t\t\t std::forward_as_tuple(&fb, MappedFrameBuffer::MapFlag::Read));\n> >\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 edf8c58e..c44a6242 100644\n> >> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> >> @@ -63,6 +63,7 @@ public:\n> >> \n> >> \tstd::unique_ptr ipa_;\n> >> \tstd::vector> mockIPABufs_;\n> >> +\tstd::vector ipaBuffers_;\n> >> };\n> >> \n> >> class VimcCameraConfiguration : public CameraConfiguration\n> >> @@ -334,6 +335,15 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> >> \tif (ret < 0)\n> >> \t\treturn ret;\n> >> \n> >> +\t/* Map the mock IPA buffers to VIMC IPA to exercise IPC code paths */\n> >> +\tunsigned int ipaBufferId = 1;\n> >> +\tfor (unsigned int i = 0; i < data->mockIPABufs_.size(); i++) {\n> >> +\t\tstd::unique_ptr &buffer = data->mockIPABufs_[i];\n> >> +\t\tbuffer->setCookie(ipaBufferId++);\n> >\n> > You can use utils::enumerate() here:\n> >\n> > \tfor (auto [i, buffer] : utils::enumerate(data->mackIPABufs_)) {\n> > \t\tbuffer->setCookie(i++);\n> >\n> >> +\t\tdata->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n> >> +\t}\n> >> +\tdata->ipa_->mapBuffers(data->ipaBuffers_);\n> >\n> > The return value should be checked.\n> \n> I didn't find mapBuffers/unmapBuffers() to return a value, in any of the \n> mojom interfaces we have today.\n> \n> I think mapBuffers() should have return value indeed, can I skip it here \n> for now, and create a patch on top which touches all the concerned IPAs \n> as well?\n\nSure.\n\n> >> +\n> >> \tret = data->ipa_->start();\n> >> \tif (ret) {\n> >> \t\tdata->video_->releaseBuffers();\n> >> @@ -354,7 +364,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> >\n> > You could use here\n> >\n> > \tfor (const std::unique_ptr &buffer : data->mockIPABufs_)\n> > \t\tids.push_back(buffer->cookie());\n> >\n> > and replace VimcCameraData::ipaBuffers_ with a local variable in\n> > PipelineHandlerVimc::start().\n> >\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 3C5FABD87D\n\tfor ;\n\tSat, 14 Aug 2021 19:51:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CD0D68892;\n\tSat, 14 Aug 2021 21:51: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 93AF060261\n\tfor ;\n\tSat, 14 Aug 2021 21:51: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 1956F3F0;\n\tSat, 14 Aug 2021 21:51:48 +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=\"ZIyvlUq4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628970708;\n\tbh=taOxua1SRMeo34u2Dd0nAxdP+pldVTVwOLmLwgZp72E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZIyvlUq4fCIFW7HBGgAjqBNoF6MYq/x0SywonZfZwZzIvrpmXzaBaJHV3dSYHEbVO\n\tynRP8XNAy+5jejXfI5ftbuM5qtFCe2lDmiT3qmaoW97Dx6QuH3JUsTUx+kB4B/q4su\n\t6tqesd4dNlyZ3QqPBB7TmVY5YHoQnPyys2Qxhm9M=","Date":"Sat, 14 Aug 2021 22:51:43 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210813144437.138005-1-umang.jain@ideasonboard.com>\n\t<20210813144437.138005-4-umang.jain@ideasonboard.com>\n\t\n\t<35205819-d96e-5fec-f7da-a135bf5c9856@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<35205819-d96e-5fec-f7da-a135bf5c9856@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: vimc: Map and unmap\n\tbuffers","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\" "}}]