[{"id":24698,"web_url":"https://patchwork.libcamera.org/comment/24698/","msgid":"","date":"2022-08-19T01:02:53","subject":"Re: [libcamera-devel] [PATCH v2 7/7] [TEST] ipa: vimc: Add Flags to\n\tparameters","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Aug 18, 2022 at 03:49:22PM +0900, Paul Elder via libcamera-devel wrote:\n> For the purpose of testing serializing/deserializing Flags in function\n> parameters, add an enum class TestFlags and Flags to some\n> function parameters, both for input and output and Signals.\n> \n> While at it, update the ipa_interface_test.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v2:\n> - use new attribute-based mojom definition for Flags\n> ---\n> include/libcamera/ipa/vimc.mojom | 14 ++++++++++++--\n> src/ipa/vimc/vimc.cpp | 21 +++++++++++++++++----\n> src/libcamera/pipeline/vimc/vimc.cpp | 16 +++++++++++++---\n> test/ipa/ipa_interface_test.cpp | 6 +++++-\n> 4 files changed, 47 insertions(+), 10 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n> index 16149787..15948d77 100644\n> --- a/include/libcamera/ipa/vimc.mojom\n> +++ b/include/libcamera/ipa/vimc.mojom\n> @@ -17,8 +17,18 @@ enum IPAOperationCode {\n> \tIPAOperationStop,\n> };\n> \n> +[Flags] enum TestFlags {\n\nThis should be TestFlag as each enumerator is a single flag. We do the\nsame in C++ code, and then create a type alias\n\n\tusing TestFlags = Flags;\n\nWe could actually try to do something similar in mojo, by defining a\ntype that aliases to Flags and repurposing the [flags] attribute to\nqualify the type definition, instead of qualifying every usage of the\nenum in structures or function parameters, but that's probably overkill.\n\n> +\tFlag1 = 0x1,\n> +\tFlag2 = 0x2,\n> +\tFlag3 = 0x4,\n> +\tFlag4 = 0x8,\n> +};\n> +\n> interface IPAVimcInterface {\n> -\tinit(libcamera.IPASettings settings, IPAOperationCode code) => (int32 ret);\n> +\tinit(libcamera.IPASettings settings,\n> +\t IPAOperationCode code,\n> +\t [Flags] TestFlags inFlags)\n> +\t=> (int32 ret, [Flags] TestFlags outFlags);\n> \n> \tconfigure(libcamera.IPACameraSensorInfo sensorInfo,\n> \t\t map streamConfig,\n> @@ -41,5 +51,5 @@ interface IPAVimcInterface {\n> };\n> \n> interface IPAVimcEventInterface {\n> -\tparamsBufferReady(uint32 bufferId);\n> +\tparamsBufferReady(uint32 bufferId, [Flags] TestFlags flags);\n> };\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index 6bf39a1c..bb4ca355 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -31,7 +31,10 @@ public:\n> \tIPAVimc();\n> \t~IPAVimc();\n> \n> -\tint init(const IPASettings &settings, const ipa::vimc::IPAOperationCode &code) override;\n> +\tint init(const IPASettings &settings,\n> +\t\t const ipa::vimc::IPAOperationCode &code,\n> +\t\t const Flags &inFlags,\n\nSame question as in 6/7, should this be passed by value ?\n\n> +\t\t Flags *outFlags) override;\n> \n> \tint start() override;\n> \tvoid stop() override;\n> @@ -65,8 +68,10 @@ IPAVimc::~IPAVimc()\n> \tif (fd_ != -1)\n> \t\t::close(fd_);\n> }\n> -\n> -int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode &code)\n> +int IPAVimc::init(const IPASettings &settings,\n> +\t\t const ipa::vimc::IPAOperationCode &code,\n> +\t\t const Flags &inFlags,\n> +\t\t Flags *outFlags)\n> {\n> \ttrace(ipa::vimc::IPAOperationInit);\n> \n> @@ -76,6 +81,13 @@ int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode\n> \n> \tLOG(IPAVimc, Debug) << \"Got opcode \" << code;\n> \n> +\tLOG(IPAVimc, Debug)\n> +\t\t<< \"Flag 2 was \"\n> +\t\t<< ((inFlags & ipa::vimc::TestFlags::Flag2) ? \"\" : \"not \")\n\nI think you can drop the inner parentheses.\n\n> +\t\t<< \"set\";\n> +\n> +\t*outFlags |= ipa::vimc::TestFlags::Flag1;\n> +\n> \tFile conf(settings.configurationFile);\n> \tif (!conf.open(File::OpenModeFlag::ReadOnly)) {\n> \t\tLOG(IPAVimc, Error) << \"Failed to open configuration file\";\n> @@ -144,7 +156,8 @@ void IPAVimc::fillParamsBuffer([[maybe_unused]] uint32_t frame, uint32_t bufferI\n> \t\treturn;\n> \t}\n> \n> -\tparamsBufferReady.emit(bufferId);\n> +\tFlags flags;\n> +\tparamsBufferReady.emit(bufferId, flags);\n> }\n> \n> void IPAVimc::initTrace()\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 983bd514..e74a8679 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -54,7 +54,7 @@ public:\n> \tint init();\n> \tint allocateMockIPABuffers();\n> \tvoid bufferReady(FrameBuffer *buffer);\n> -\tvoid paramsBufferReady(unsigned int id);\n> +\tvoid paramsBufferReady(unsigned int id, const Flags &flags);\n> \n> \tMediaDevice *media_;\n> \tstd::unique_ptr sensor_;\n> @@ -471,7 +471,16 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \tdata->ipa_->paramsBufferReady.connect(data.get(), &VimcCameraData::paramsBufferReady);\n> \n> \tstd::string conf = data->ipa_->configurationFile(\"vimc.conf\");\n> -\tdata->ipa_->init(IPASettings{ conf, data->sensor_->model() }, ipa::vimc::IPAOperationInit);\n> +\tFlags inFlags;\n> +\tFlags outFlags;\n> +\tinFlags |= ipa::vimc::TestFlags::Flag2;\n\nThis could be set when construcing inFlags.\n\n> +\tdata->ipa_->init(IPASettings{ conf, data->sensor_->model() },\n> +\t\t\t ipa::vimc::IPAOperationInit, inFlags, &outFlags);\n> +\n> +\tLOG(VIMC, Debug)\n> +\t\t<< \"Flag 1 was \"\n> +\t\t<< ((outFlags & ipa::vimc::TestFlags::Flag1) ? \"\" : \"not \")\n\nYou can drop the inner parentheses here too.\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t<< \"set\";\n> \n> \t/* Create and register the camera. */\n> \tstd::set streams{ &data->stream_ };\n> @@ -608,7 +617,8 @@ int VimcCameraData::allocateMockIPABuffers()\n> \treturn video_->exportBuffers(kBufCount, &mockIPABufs_);\n> }\n> \n> -void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id)\n> +void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id,\n> +\t\t\t\t [[maybe_unused]] const Flags &flags)\n> {\n> }\n> \n> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\n> index b9fa15cd..47c7352b 100644\n> --- a/test/ipa/ipa_interface_test.cpp\n> +++ b/test/ipa/ipa_interface_test.cpp\n> @@ -106,7 +106,11 @@ protected:\n> \n> \t\t/* Test initialization of IPA module. */\n> \t\tstd::string conf = ipa_->configurationFile(\"vimc.conf\");\n> -\t\tint ret = ipa_->init(IPASettings{ conf, \"vimc\" }, ipa::vimc::IPAOperationInit);\n> +\t\tFlags inFlags;\n> +\t\tFlags outFlags;\n> +\t\tint ret = ipa_->init(IPASettings{ conf, \"vimc\" },\n> +\t\t\t\t ipa::vimc::IPAOperationInit,\n> +\t\t\t\t inFlags, &outFlags);\n> \t\tif (ret < 0) {\n> \t\t\tcerr << \"IPA interface init() failed\" << endl;\n> \t\t\treturn TestFail;","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 A4EAABE173\n\tfor ;\n\tFri, 19 Aug 2022 01:02:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC7EC61FC0;\n\tFri, 19 Aug 2022 03:02:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 536D361FA8\n\tfor ;\n\tFri, 19 Aug 2022 03:02:57 +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 AE8CD8B;\n\tFri, 19 Aug 2022 03:02:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660870978;\n\tbh=8Cya/pXzyg73YJTfkX32rPf7ohYB/cbW/3/OZ4mel1M=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=StRSNqLvW23LT4qp1ynCrS7bDypA/VhCeYbKSOQrgc8Noq7ZF8FcWJCo8uef7heNI\n\t1eDww+zNWmHE8LY/m38i7JNNBsKYW+WUTVRrB36sGfGh2OapwkKMD0N75LmC3KAX31\n\t12jZzE9Oml3f097jq9jtQDYFiEOKIoeAexxHXvQpYG1Gl72Q3Twl7huMAjwNZCpczP\n\tPbrNfu1DJb5L7Vg9ElSnvabWfpDdxgLCvessCWOwcbwjnyCubTsnVBRSwig+kTsb5m\n\tjcTonAYEmASqr+JEi8h4xAje3wk6TdjkGHb+4ydYv7tXRXEHTzU0YYNVw9uOlC7YEz\n\tL0UYvaRSDvWXA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660870976;\n\tbh=8Cya/pXzyg73YJTfkX32rPf7ohYB/cbW/3/OZ4mel1M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ga2J5ZLYvnqdopRodkNHDSx0KZlNYk834o72pE7hgo9SFcZL1u7yz+0HOprYuXpSV\n\totXdD2bq6aIppGa1LLNRuL7whHRfStUElCYcHGF3roOnE3yWgPt2LtK01+2TcwBigd\n\twybzTXPAuOkiaJ1QQTfiyHZEumf+qBe51ciJWhLo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ga2J5ZLY\"; dkim-atps=neutral","Date":"Fri, 19 Aug 2022 04:02:53 +0300","To":"Paul Elder ","Message-ID":"","References":"<20220818064923.2573060-1-paul.elder@ideasonboard.com>\n\t<20220818064923.2573060-8-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220818064923.2573060-8-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] [TEST] ipa: vimc: Add Flags to\n\tparameters","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]