[{"id":18790,"web_url":"https://patchwork.libcamera.org/comment/18790/","msgid":"","date":"2021-08-14T01:09:11","subject":"Re: [libcamera-devel] [PATCH v2 4/4] ipa: vimc: Send and retrieve\n\tFrameBuffers from IPA","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:37PM +0530, Umang Jain wrote:\n> Plumb through VIMC mojo interface to enable buffers passing.\n> Since VIMC is a virtual test driver and does not have actual\n> IPA buffers (parameters and statistics), we can only pass them\n> as is, to and from the IPA. The buffers used here are mock\n> FrameBuffers which are dmabuf backed(in other words, mmap()able\n\ns/backed/backed /\n\n> through MappedFramebuffer inside the IPA).\n> \n> This commits shows:\n> - Passing and filling the parameter buffer from the pipeline handler to\n> the IPA.\n> - Passing request controls ControlList to the IPA.\n> \n> Any tests using VIMC will now loop in the IPA paths. Any tests running\n> in isolated mode will help us to test IPA IPC code paths especially\n> around (de)serialization of data passing from pipeline handlers to the\n> IPA. Future IPA interface tests can simply extend the VIMC mojom\n> interface to achieve/test a specific use case as required.\n\nThe code here looks fine overall, but of course it doesn't do much,\nbecause there's not much to do. I think that the main reason for this\npatch series is to test buffer sharing with IPA and fd passing over IPC.\nFrom that point of view, passing buffer ids as integers at runtime\ndoesn't really improve test coverage. On the other hand, I think it can\nbe useful to exercise IPA communication at runtime, for instance to\ncatch performance issues. I'm not entirely sure, however, that we would\nbe able to catch such performance issues with vimc, but we may be able\nto catch bad bugs still.\n\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/ipa/vimc.mojom | 14 +++++++++++++-\n> src/ipa/vimc/vimc.cpp | 19 +++++++++++++++++++\n> src/libcamera/pipeline/vimc/vimc.cpp | 11 +++++++++++\n> 3 files changed, 43 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n> index 8cb240d3..85a2d8e4 100644\n> --- a/include/libcamera/ipa/vimc.mojom\n> +++ b/include/libcamera/ipa/vimc.mojom\n> @@ -29,8 +29,20 @@ interface IPAVimcInterface {\n> \n> \tmapBuffers(array buffers);\n> \tunmapBuffers(array ids);\n> +\n> +\t/*\n> +\t * VIMC being a virtual test driver does not have actual parameters\n> +\t * and statistics buffers. However, VIMC pipeline creates mock IPA\n> +\t * buffers and map it to IPA.\n\nTechnically speaking, it's not because vimc is a driver for a virtual\ndevice, it's because it doesn't implement that feature. I would just\nwrite \"VIMC does not have ...\". Same comment for other patches in the\nseries, including commit message.\n\n> +\t *\n> +\t * Since, we are not working with actual IPA buffers here,\n\nThe buffers are real, and they're passed to the IPA, so they're IPA\nbuffers. The point of this series is to implement operations that are\nnot required by the driver but that mimick how other pipeline handler\noperate, for testing purpose. How about this ?\n\n\t/*\n\t * The vimc driver doesn't use parameters buffers. To maximize coverage\n\t * of unit tests that rely on the VIMC pipeline handler, we still define\n\t * interface functions that mimick how other pipeline handlers typically\n\t * handle parameters at runtime.\n\t */\n\nMaybe commit messages could be update accordingly.\n\n> +\t * following are mostly stub functions, introduced to leverage IPA\n> +\t * IPC tests.\n> +\t */\n> +\t[async] fillParams(uint32 frame, uint32 bufferId);\n> +\t[async] processControls(uint32 frame, libcamera.ControlList controls);\n> };\n> \n> interface IPAVimcEventInterface {\n> -\tdummyEvent(uint32 val);\n> +\tparamsFilled(uint32 bufferId);\n> };\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index 082b2db7..be189181 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -43,6 +43,9 @@ public:\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> \n> +\tvoid fillParams(uint32_t frame, uint32_t bufferId) override;\n> +\tvoid processControls(uint32_t frame, const ControlList &controls) override;\n> +\n> private:\n> \tvoid initTrace();\n> \tvoid trace(enum ipa::vimc::IPAOperationCode operation);\n> @@ -125,6 +128,22 @@ void IPAVimc::unmapBuffers(const std::vector &ids)\n> \t}\n> }\n> \n> +void IPAVimc::fillParams([[maybe_unused]] uint32_t frame, uint32_t bufferId)\n> +{\n> +\tauto it = buffers_.find(bufferId);\n> +\tif (it == buffers_.end()) {\n> +\t\tLOG(IPAVimc, Error) << \"Could not find parameter buffer\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tparamsFilled.emit(bufferId);\n> +}\n> +\n> +void IPAVimc::processControls([[maybe_unused]] uint32_t frame,\n> +\t\t\t [[maybe_unused]] const ControlList &controls)\n> +{\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 c44a6242..bfd545c8 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -52,6 +52,7 @@ public:\n> \tint init();\n> \tint allocateMockIPABuffers();\n> \tvoid bufferReady(FrameBuffer *buffer);\n> +\tvoid paramsFilled(unsigned int id);\n> \n> \tMediaDevice *media_;\n> \tstd::unique_ptr sensor_;\n> @@ -433,6 +434,8 @@ int PipelineHandlerVimc::queueRequestDevice(Camera *camera, Request *request)\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> +\tdata->ipa_->processControls(request->sequence(), request->controls());\n> +\n> \treturn 0;\n> }\n> \n> @@ -466,6 +469,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n> \t}\n> \n> +\tdata->ipa_->paramsFilled.connect(data.get(), &VimcCameraData::paramsFilled);\n> +\n> \tstd::string conf = data->ipa_->configurationFile(\"vimc.conf\");\n> \tdata->ipa_->init(IPASettings{ conf, data->sensor_->model() });\n> \n> @@ -570,6 +575,8 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> \n> \tpipe_->completeBuffer(request, buffer);\n> \tpipe_->completeRequest(request);\n> +\n> +\tipa_->fillParams(request->sequence(), mockIPABufs_[0]->cookie());\n\nWe're always using the same buffer, but that's not an issue for now as\nwe don't actually use the buffer anywhere. Down the line, I could\nimagine storing control values in the buffer, and applying those\ncontrols in the pipeline handler, but that's for latter.\n\n> }\n> \n> int VimcCameraData::allocateMockIPABuffers()\n> @@ -591,6 +598,10 @@ int VimcCameraData::allocateMockIPABuffers()\n> \treturn video_->exportBuffers(bufCount, &mockIPABufs_);\n> }\n> \n> +void VimcCameraData::paramsFilled([[maybe_unused]] unsigned int id)\n> +{\n> +}\n> +\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)\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 AAD92C3240\n\tfor ;\n\tSat, 14 Aug 2021 01:09:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 149ED6888D;\n\tSat, 14 Aug 2021 03:09:20 +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 2E28968823\n\tfor ;\n\tSat, 14 Aug 2021 03:09:18 +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 9BB8F3E5;\n\tSat, 14 Aug 2021 03:09:17 +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=\"DF6l3haM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628903357;\n\tbh=nN3ws3aF0U73YCmeCHAG9k/5Hhq//9NEGGxsn3/xhoY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DF6l3haMroPRpZlmNJWhVZL17ACU3lABpdDyVgs6AbttbtU5hPKaV2ABAFeAOicnX\n\t7hqicAfHjUVlImwgtGqhGw6bxLBV9C0xJrGbvGEppW83lbJ9NwmrQDAndzrlCValbX\n\tl6s80Fm1rklRrEf5rrHLrFoLA+CjbSnLkx0CALkQ=","Date":"Sat, 14 Aug 2021 04:09:11 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210813144437.138005-1-umang.jain@ideasonboard.com>\n\t<20210813144437.138005-5-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210813144437.138005-5-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] ipa: vimc: Send and retrieve\n\tFrameBuffers from IPA","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\" "}}]