[{"id":1689,"web_url":"https://patchwork.libcamera.org/comment/1689/","msgid":"<20190523201542.GD21601@pendragon.ideasonboard.com>","date":"2019-05-23T20:15:42","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: ipa_module: add\n\taquired attribute","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, May 23, 2019 at 12:42:07PM -0400, Paul Elder wrote:\n> The IPAManager will be designed like an enumerator, and IPA modules\n> cannot be used by multiple pipelines at once. Add an aquired attribute\n> to IPAModule, and corresponding getter and setters.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v2:\n> - renamed acquired() to isAcquired(), to match isValid()\n> - added documentation\n> \n> src/libcamera/include/ipa_module.h | 5 +++\n> src/libcamera/ipa_module.cpp | 62 ++++++++++++++++++++++++++----\n> 2 files changed, 60 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> index a4c6dbd..58faeca 100644\n> --- a/src/libcamera/include/ipa_module.h\n> +++ b/src/libcamera/include/ipa_module.h\n> @@ -22,11 +22,16 @@ public:\n> \n> \tconst struct IPAModuleInfo &info() const;\n> \n> +\tbool isAcquired() const;\n> +\tbool acquire();\n> +\tvoid release();\n> +\n> private:\n> \tstruct IPAModuleInfo info_;\n> \n> \tstd::string libPath_;\n> \tbool valid_;\n> +\tbool acquired_;\n> \n> \tint loadIPAModuleInfo();\n> };\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 86cbe71..4922e3e 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -176,14 +176,17 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n> * This structure contains the information of an IPA module. It is loaded,\n> * read, and validated before anything else is loaded from the shared object.\n> *\n\nAh, here is the documentation :-) Please move it to patch 1/5.\n\n> - * \\var IPAModuleInfo::name\n> - * \\brief The name of the IPA module\n> + * \\var IPAModuleInfo::ipaAPIVersion\n> + * \\brief The IPA API version that the IPA module was made with\n\nYou should also describe how the version is constructed. For this field,\nI believe an plain integer is fine, and we will bump it every time the\nIPAModuleInfo structure layout gets modified. The IPA module shall\nreport here the version that it was built for, and the ipa_module_info.h\nheader shall contain a macro with the current version number (starting\nat 1).\n\n> *\n> - * \\var IPAModuleInfo::version\n> - * \\brief The version of the IPA module\n> + * \\var IPAModuleInfo::pipelineVersion\n> + * \\brief The pipeline version that the IPA module is for\n\nFor this field, however, I think we need a major.minor type of version.\nI'm however not sure if the IPA module should report the version it has\nbeen built for, or the minimal version it needs. A match would occur if\nthe major is identical, and the minor at least the requested one. In the\nother direction the module would report an exact version, and the\npipeline would request a minimum version. What do you think would be\nbest ?\n\n> *\n> - * \\todo abi compatability version\n> - * \\todo pipeline compatability matcher\n> + * \\var IPAModuleInfo::pipelineName\n> + * \\brief The name of the pipeline that the IPA module is for\n> + *\n\nPlease describe this in more details, at least stating where the\npipeline name comes from (and it should be pipeline handler, not\npipeline).\n\n> + * \\var IPAModuleInfo::name\n> + * \\brief The name of the IPA module\n> */\n> \n> /**\n> @@ -212,7 +215,7 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n> * IPAModule instance to verify the validity of the IPAModule.\n> */\n> IPAModule::IPAModule(const std::string &libPath)\n> -\t: libPath_(libPath), valid_(false)\n> +\t: libPath_(libPath), valid_(false), acquired_(false)\n> {\n> \tif (loadIPAModuleInfo() < 0)\n> \t\treturn;\n> @@ -289,4 +292,49 @@ const struct IPAModuleInfo &IPAModule::info() const\n> \treturn info_;\n> }\n> \n> +/**\n> + * \\brief Check if IPA module is in use\n> + *\n> + * \\return true if the IPA module has been claimed for exclusive use, or\n> + * false if it is available\n> + * \\sa acquire(), release()\n> + */\n> +bool IPAModule::isAcquired() const\n> +{\n> +\treturn acquired_;\n> +}\n> +\n> +/**\n> + * \\brief Claim an IPA module for exclusive use\n> + *\n> + * Each IPA module is meant to be used by only one pipeline handler.\n> + * IPA modules will be acquired through the IPAManager, which will\n> + * use this method to claim an IPA module before returning it, and will\n> + * skip over already claimed IPA modules.\n> + *\n> + * When the IPA module is not needed anymore, the release() method should\n> + * be called.\n> + *\n> + * \\return true if the IPA module was successfully claimed, or false if\n> + * was already claimed\n> + * \\sa isAcquired(), release()\n> + */\n> +bool IPAModule::acquire()\n> +{\n> +\tif (acquired_)\n> +\t\treturn false;\n> +\n> +\tacquired_ = true;\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Release an IPA module previously claimed for exclusive use\n> + * \\sa acquire(), isAcquired()\n> + */\n> +void IPAModule::release()\n> +{\n> +\tacquired_ = false;\n> +}\n> +\n\nI don't think this will work. If we have two instances of a pipeline\nhandler because the system contains two instances of the same device,\nthey should both work with the same IPA module. What will then happen is\nthat the factory method exported by the IPA module will be called twice\nto create two instances of a yet-to-be-defined IPA object, one for each\npipeline handler. I believe you can thus drop those three functions.\n\n> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["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 359DC60E9A\n\tfor ;\n\tThu, 23 May 2019 22:16:01 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8E0665A9;\n\tThu, 23 May 2019 22:16:00 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558642560;\n\tbh=8FRfPBcvjI3IzVBCpIfJRWiYvp8xXogqKAjtMRhpRg8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dsETfqjlwN0daXfidEMbjV6bO+CNhVLrF3a9zm8zjt18O6wpjOI23r5dydtUCcoGI\n\tjbdKCu0rvDqgkFmlLWMuvzxRf8+gbrjPVWf+M7Yk8ggFbR4s3vW0Rrw5enjUJMLIiI\n\togOWgranMPo48nNw2pV1YkYhd4lhHWTMwh9UVGbk=","Date":"Thu, 23 May 2019 23:15:42 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190523201542.GD21601@pendragon.ideasonboard.com>","References":"<20190523164210.2105-1-paul.elder@ideasonboard.com>\n\t<20190523164210.2105-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190523164210.2105-3-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: ipa_module: add\n\taquired attribute","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 23 May 2019 20:16:01 -0000"}}]