[{"id":24284,"web_url":"https://patchwork.libcamera.org/comment/24284/","msgid":"<792cb34d-1a86-ecf6-7d27-e499375d866a@ideasonboard.com>","date":"2022-08-02T04:46:09","subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Move ControlInfoMap\n\tto IPA module","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch\n\nOn 8/2/22 05:43, Laurent Pinchart via libcamera-devel wrote:\n> Currently the pipeline handler advertises controls handled by the IPA\n> from a ControlInfoMap it manually constructs. This is wrong, as the IPA\n> module is the component that knows what controls it supports. Fix this\n> by moving the ControlInfoMap construction to the IPA module, and pass it\n> to the pipeline handler as a return value from the IPA init() function.\n>\n> Signed-off-by: Laurent Pinchart \n\n\nReviewed-by: Umang Jain \n\n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> src/ipa/rkisp1/rkisp1.cpp | 30 ++++++++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++----------------------\n> 3 files changed, 30 insertions(+), 33 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index e35373852b7c..eaf3955e4096 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -11,7 +11,7 @@ import \"include/libcamera/ipa/core.mojom\";\n> interface IPARkISP1Interface {\n> \tinit(libcamera.IPASettings settings,\n> \t uint32 hwRevision)\n> -\t\t=> (int32 ret);\n> +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> \tstart() => (int32 ret);\n> \tstop();\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index e5010f6ab629..52f7143980fe 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -46,7 +46,8 @@ namespace ipa::rkisp1 {\n> class IPARkISP1 : public IPARkISP1Interface, public Module\n> {\n> public:\n> -\tint init(const IPASettings &settings, unsigned int hwRevision) override;\n> +\tint init(const IPASettings &settings, unsigned int hwRevision,\n> +\t\t ControlInfoMap *ipaControls) override;\n> \tint start() override;\n> \tvoid stop() override {}\n> \n> @@ -89,12 +90,27 @@ private:\n> \tstruct IPAContext context_;\n> };\n> \n> +namespace {\n> +\n> +/* List of controls handled by the RkISP1 IPA */\n> +const ControlInfoMap::Map rkisp1Controls{\n> +\t{ &controls::AeEnable, ControlInfo(false, true) },\n> +\t{ &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n> +\t{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> +\t{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n> +\t{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> +\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> +};\n> +\n> +} /* namespace */\n> +\n> std::string IPARkISP1::logPrefix() const\n> {\n> \treturn \"rkisp1\";\n> }\n> \n> -int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> +int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> +\t\t ControlInfoMap *ipaControls)\n> {\n> \t/* \\todo Add support for other revisions */\n> \tswitch (hwRevision) {\n> @@ -156,7 +172,15 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\treturn createAlgorithms(context_, (*data)[\"algorithms\"]);\n> +\tint ret = createAlgorithms(context_, (*data)[\"algorithms\"]);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Return the controls handled by the IPA. */\n> +\tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> +\n> +\treturn 0;\n> }\n> \n> int IPARkISP1::start()\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index de687f4dc6b0..932873329e89 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -344,7 +344,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \t\tipaTuningFile = std::string(configFromEnv);\n> \t}\n> \n> -\tint ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision);\n> +\tint ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> +\t\t\t &controlInfo_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> @@ -967,34 +968,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \t\tstd::make_unique(this, &mainPath_,\n> \t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> \n> -\tControlInfoMap::Map ctrls;\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::Sharpness),\n> -\t\t std::forward_as_tuple(0.0f, 10.0f, 1.0f));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::Brightness),\n> -\t\t std::forward_as_tuple(-1.0f, 0.993f));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::Contrast),\n> -\t\t std::forward_as_tuple(0.0f, 1.993f));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::Saturation),\n> -\t\t std::forward_as_tuple(0.0f, 1.993f));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::draft::NoiseReductionMode),\n> -\t\t std::forward_as_tuple(controls::draft::NoiseReductionModeValues));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::AeEnable),\n> -\t\t std::forward_as_tuple(false, true));\n> -\n> -\tdata->controlInfo_ = ControlInfoMap(std::move(ctrls),\n> -\t\t\t\t\t controls::controls);\n> -\n> \tdata->sensor_ = std::make_unique(sensor);\n> \tret = data->sensor_->init();\n> \tif (ret)","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 5591EBE173\n\tfor ;\n\tTue, 2 Aug 2022 04:46:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD09763312;\n\tTue, 2 Aug 2022 06:46:16 +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 7CA9861FAE\n\tfor ;\n\tTue, 2 Aug 2022 06:46:15 +0200 (CEST)","from [IPV6:2401:4900:1f3f:85c2:5ee8:5bb8:aca7:5517] (unknown\n\t[IPv6:2401:4900:1f3f:85c2:5ee8:5bb8:aca7:5517])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8B27025B;\n\tTue, 2 Aug 2022 06:46:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659415576;\n\tbh=TMV1Qz6EjHANSB6a4JWkTOValfUZuIoobKtoTCOUqEA=;\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:\n\tFrom;\n\tb=G9lVYl/wc1EY4RSfkZk19xn863bC+JsZZmEMu+hSBJzENx2nw1QE/g7Q/UmdWrvVY\n\tFPuz/RrEx2+hF/Mf6XYmUhHqKq81NqddW1D+Es/khIul10cq8+oB7keqr/FKxDV05e\n\t8VdIrTPyfit4+ydKh8CsTzus146jNSqwul7pcOf6OdjK9TZcubOrlTKttAn4mgKvgE\n\tJgYtei2RLaTL0xQERvyTI5zMsDBZE5f3f+UPSRCJNth9zUG9WiRomjZU/oHyDfZCyJ\n\tfloSSJh/wFyyyocT+lfVNzrgajGw7M+FL/+IoV+zMALK/lr0EhPAkOHOFjtTrMFGl+\n\tUWTi7BmFZKKdw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659415575;\n\tbh=TMV1Qz6EjHANSB6a4JWkTOValfUZuIoobKtoTCOUqEA=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=mVJKfJODQZ/Z+6ncJO1QBOXWNyMmtVWADGodc2Wrz1A+1wL9RwXcc6y5rAE19FOup\n\t1TEsREpQOo/whnfsdErteMAy2Iql68lhVpkHF1qs+t8KQ0jaLCUsI1O/EkRCPrgtVx\n\t7ikV09+jL2ySoiqcw31CLAVgTpFGo4lSAQ6YPpk8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mVJKfJOD\"; dkim-atps=neutral","Message-ID":"<792cb34d-1a86-ecf6-7d27-e499375d866a@ideasonboard.com>","Date":"Tue, 2 Aug 2022 10:16:09 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220802001330.17073-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20220802001330.17073-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Move ControlInfoMap\n\tto IPA module","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24348,"web_url":"https://patchwork.libcamera.org/comment/24348/","msgid":"<20220803134557.GF311202@pyrite.rasen.tech>","date":"2022-08-03T13:45:57","subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Move ControlInfoMap\n\tto IPA module","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Tue, Aug 02, 2022 at 03:13:30AM +0300, Laurent Pinchart wrote:\n> Currently the pipeline handler advertises controls handled by the IPA\n> from a ControlInfoMap it manually constructs. This is wrong, as the IPA\n> module is the component that knows what controls it supports. Fix this\n> by moving the ControlInfoMap construction to the IPA module, and pass it\n> to the pipeline handler as a return value from the IPA init() function.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> src/ipa/rkisp1/rkisp1.cpp | 30 ++++++++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++----------------------\n> 3 files changed, 30 insertions(+), 33 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index e35373852b7c..eaf3955e4096 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -11,7 +11,7 @@ import \"include/libcamera/ipa/core.mojom\";\n> interface IPARkISP1Interface {\n> \tinit(libcamera.IPASettings settings,\n> \t uint32 hwRevision)\n> -\t\t=> (int32 ret);\n> +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> \tstart() => (int32 ret);\n> \tstop();\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index e5010f6ab629..52f7143980fe 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -46,7 +46,8 @@ namespace ipa::rkisp1 {\n> class IPARkISP1 : public IPARkISP1Interface, public Module\n> {\n> public:\n> -\tint init(const IPASettings &settings, unsigned int hwRevision) override;\n> +\tint init(const IPASettings &settings, unsigned int hwRevision,\n> +\t\t ControlInfoMap *ipaControls) override;\n> \tint start() override;\n> \tvoid stop() override {}\n> \n> @@ -89,12 +90,27 @@ private:\n> \tstruct IPAContext context_;\n> };\n> \n> +namespace {\n> +\n> +/* List of controls handled by the RkISP1 IPA */\n> +const ControlInfoMap::Map rkisp1Controls{\n> +\t{ &controls::AeEnable, ControlInfo(false, true) },\n> +\t{ &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n> +\t{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> +\t{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n> +\t{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> +\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> +};\n> +\n> +} /* namespace */\n> +\n> std::string IPARkISP1::logPrefix() const\n> {\n> \treturn \"rkisp1\";\n> }\n> \n> -int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> +int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> +\t\t ControlInfoMap *ipaControls)\n> {\n> \t/* \\todo Add support for other revisions */\n> \tswitch (hwRevision) {\n> @@ -156,7 +172,15 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\treturn createAlgorithms(context_, (*data)[\"algorithms\"]);\n> +\tint ret = createAlgorithms(context_, (*data)[\"algorithms\"]);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Return the controls handled by the IPA. */\n> +\tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> +\n> +\treturn 0;\n> }\n> \n> int IPARkISP1::start()\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index de687f4dc6b0..932873329e89 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -344,7 +344,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \t\tipaTuningFile = std::string(configFromEnv);\n> \t}\n> \n> -\tint ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision);\n> +\tint ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> +\t\t\t &controlInfo_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> @@ -967,34 +968,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \t\tstd::make_unique(this, &mainPath_,\n> \t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> \n> -\tControlInfoMap::Map ctrls;\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::Sharpness),\n> -\t\t std::forward_as_tuple(0.0f, 10.0f, 1.0f));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::Brightness),\n> -\t\t std::forward_as_tuple(-1.0f, 0.993f));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::Contrast),\n> -\t\t std::forward_as_tuple(0.0f, 1.993f));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::Saturation),\n> -\t\t std::forward_as_tuple(0.0f, 1.993f));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::draft::NoiseReductionMode),\n> -\t\t std::forward_as_tuple(controls::draft::NoiseReductionModeValues));\n> -\n> -\tctrls.emplace(std::piecewise_construct,\n> -\t\t std::forward_as_tuple(&controls::AeEnable),\n> -\t\t std::forward_as_tuple(false, true));\n> -\n> -\tdata->controlInfo_ = ControlInfoMap(std::move(ctrls),\n> -\t\t\t\t\t controls::controls);\n> -\n> \tdata->sensor_ = std::make_unique(sensor);\n> \tret = data->sensor_->init();\n> \tif (ret)\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 0D39FC3272\n\tfor ;\n\tWed, 3 Aug 2022 13:46:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B52C63310;\n\tWed, 3 Aug 2022 15:46: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 897B6603E6\n\tfor ;\n\tWed, 3 Aug 2022 15:46:05 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BAA108B;\n\tWed, 3 Aug 2022 15:46:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659534366;\n\tbh=DGM+XHSEnUfC0iYf2xmbCr7SPDRi67lk/IX1WpHyWIc=;\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=VZYZn7FA/bQnB3UNlJB1iAHrR+2bQzeXESoluM3PzPBe4mwXqPf3kuD3egZ30/qd1\n\tOItVpPG0R5H026eKSCEojfUCVZay0JzewZzIqGfyLkxOdrWpyKIdlDEDgWNevBtDBQ\n\trHHzrhb+HYzwpWREO8x4lAmqGVE9uShd3PT0LDNSeT2ovkZ4TtxrwuACOynGb3Xiow\n\tH9GfjVOlQ5RpqZ+aPTBRjjN/sEL0EJ3kt27ZCV3N2jcAB0JpOsyiiHoTPWaO7obr6K\n\tLWz0D1hLpJg+q8Mc1vQgXC/R9nTBxk8zNSqv68NZ8f3nmWNmIOmXWbEmxtAeAlItnq\n\t5dowY2BAP43fA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659534365;\n\tbh=DGM+XHSEnUfC0iYf2xmbCr7SPDRi67lk/IX1WpHyWIc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aK9sdgxg5FvycHq6WNBeKr11zFsjg3zcnQ0sAR9lUWCxBFHLNtBfe5LoFwaTMtoTe\n\tCmtZUXBU8ntJ6gv/dfqc/YvN5kwHkPjALEHkNgh+zlvuLh5KXLeydVzT9pOzEcXXMo\n\tgS23n0Bd6asFeKFBvgyESCS5GMoNfif2nR8UTQ6A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"aK9sdgxg\"; dkim-atps=neutral","Date":"Wed, 3 Aug 2022 22:45:57 +0900","To":"Laurent Pinchart ","Message-ID":"<20220803134557.GF311202@pyrite.rasen.tech>","References":"<20220802001330.17073-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220802001330.17073-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Move ControlInfoMap\n\tto IPA module","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24358,"web_url":"https://patchwork.libcamera.org/comment/24358/","msgid":"","date":"2022-08-03T16:02:00","subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Move ControlInfoMap\n\tto IPA module","submitter":{"id":123,"url":"https://patchwork.libcamera.org/api/people/123/","name":"Florian Sylvestre","email":"fsylvestre@baylibre.com"},"content":"Hi Laurent,\n\nOn Wed, 3 Aug 2022 at 15:46, wrote:\n>\n> On Tue, Aug 02, 2022 at 03:13:30AM +0300, Laurent Pinchart wrote:\n> > Currently the pipeline handler advertises controls handled by the IPA\n> > from a ControlInfoMap it manually constructs. This is wrong, as the IPA\n> > module is the component that knows what controls it supports. Fix this\n> > by moving the ControlInfoMap construction to the IPA module, and pass it\n> > to the pipeline handler as a return value from the IPA init() function.\n> >\n> > Signed-off-by: Laurent Pinchart \n>\n> Reviewed-by: Paul Elder \nReviewed-by: Florian Sylvestre \n>\n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 2 +-\n> > src/ipa/rkisp1/rkisp1.cpp | 30 ++++++++++++++++++++---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++----------------------\n> > 3 files changed, 30 insertions(+), 33 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index e35373852b7c..eaf3955e4096 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -11,7 +11,7 @@ import \"include/libcamera/ipa/core.mojom\";\n> > interface IPARkISP1Interface {\n> > init(libcamera.IPASettings settings,\n> > uint32 hwRevision)\n> > - => (int32 ret);\n> > + => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > start() => (int32 ret);\n> > stop();\n> >\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index e5010f6ab629..52f7143980fe 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -46,7 +46,8 @@ namespace ipa::rkisp1 {\n> > class IPARkISP1 : public IPARkISP1Interface, public Module\n> > {\n> > public:\n> > - int init(const IPASettings &settings, unsigned int hwRevision) override;\n> > + int init(const IPASettings &settings, unsigned int hwRevision,\n> > + ControlInfoMap *ipaControls) override;\n> > int start() override;\n> > void stop() override {}\n> >\n> > @@ -89,12 +90,27 @@ private:\n> > struct IPAContext context_;\n> > };\n> >\n> > +namespace {\n> > +\n> > +/* List of controls handled by the RkISP1 IPA */\n> > +const ControlInfoMap::Map rkisp1Controls{\n> > + { &controls::AeEnable, ControlInfo(false, true) },\n> > + { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n> > + { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> > + { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n> > + { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> > + { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> > +};\n> > +\n> > +} /* namespace */\n> > +\n> > std::string IPARkISP1::logPrefix() const\n> > {\n> > return \"rkisp1\";\n> > }\n> >\n> > -int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> > +int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > + ControlInfoMap *ipaControls)\n> > {\n> > /* \\todo Add support for other revisions */\n> > switch (hwRevision) {\n> > @@ -156,7 +172,15 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> > return -EINVAL;\n> > }\n> >\n> > - return createAlgorithms(context_, (*data)[\"algorithms\"]);\n> > + int ret = createAlgorithms(context_, (*data)[\"algorithms\"]);\n> > + if (ret)\n> > + return ret;\n> > +\n> > + /* Return the controls handled by the IPA. */\n> > + ControlInfoMap::Map ctrlMap = rkisp1Controls;\n> > + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> > +\n> > + return 0;\n> > }\n> >\n> > int IPARkISP1::start()\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index de687f4dc6b0..932873329e89 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -344,7 +344,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > ipaTuningFile = std::string(configFromEnv);\n> > }\n> >\n> > - int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision);\n> > + int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > + &controlInfo_);\n> > if (ret < 0) {\n> > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > return ret;\n> > @@ -967,34 +968,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > std::make_unique(this, &mainPath_,\n> > hasSelfPath_ ? &selfPath_ : nullptr);\n> >\n> > - ControlInfoMap::Map ctrls;\n> > - ctrls.emplace(std::piecewise_construct,\n> > - std::forward_as_tuple(&controls::Sharpness),\n> > - std::forward_as_tuple(0.0f, 10.0f, 1.0f));\n> > -\n> > - ctrls.emplace(std::piecewise_construct,\n> > - std::forward_as_tuple(&controls::Brightness),\n> > - std::forward_as_tuple(-1.0f, 0.993f));\n> > -\n> > - ctrls.emplace(std::piecewise_construct,\n> > - std::forward_as_tuple(&controls::Contrast),\n> > - std::forward_as_tuple(0.0f, 1.993f));\n> > -\n> > - ctrls.emplace(std::piecewise_construct,\n> > - std::forward_as_tuple(&controls::Saturation),\n> > - std::forward_as_tuple(0.0f, 1.993f));\n> > -\n> > - ctrls.emplace(std::piecewise_construct,\n> > - std::forward_as_tuple(&controls::draft::NoiseReductionMode),\n> > - std::forward_as_tuple(controls::draft::NoiseReductionModeValues));\n> > -\n> > - ctrls.emplace(std::piecewise_construct,\n> > - std::forward_as_tuple(&controls::AeEnable),\n> > - std::forward_as_tuple(false, true));\n> > -\n> > - data->controlInfo_ = ControlInfoMap(std::move(ctrls),\n> > - controls::controls);\n> > -\n> > data->sensor_ = std::make_unique(sensor);\n> > ret = data->sensor_->init();\n> > if (ret)\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 4C49FBE173\n\tfor ;\n\tWed, 3 Aug 2022 16:02:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AE1D63310;\n\tWed, 3 Aug 2022 18:02:14 +0200 (CEST)","from mail-pl1-x62f.google.com (mail-pl1-x62f.google.com\n\t[IPv6:2607:f8b0:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED196603E6\n\tfor ;\n\tWed, 3 Aug 2022 18:02:12 +0200 (CEST)","by mail-pl1-x62f.google.com with SMTP id v18so16779647plo.8\n\tfor ;\n\tWed, 03 Aug 2022 09:02:12 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659542534;\n\tbh=ots8KoNlvGO8XDdHzb1yEPKKGrxPrVdL0uDjwuXFrmc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vEUBiHlyAXr9hQHiL5t4IeOxuCMqmv5Cx8NagTBx+YWiPZ/VPW+p6oO6pQ33uzegv\n\tJt1u6sy34Ji+PMDHoExSrcuJtI9oUXGejt84DrbyIMmELFVbMCgfp89hRP0lDnX5ho\n\tJ5hnaiTBzv8kZ4dedTG1fva7hXxNclqn/FawRgTM6uFkbkXE41YEEDPuSWN/yjnTPf\n\tR7LgZnpOz1XCXjyVwcuiKK5RggHqVe8obnxACpb5pWNT1DFcOv7IlsHCkETkkyvnp3\n\tjfqf4GSEn1uK8Jq+wWOnE+kpng8Mfo58tEPNYdaCdIGypp/p1UNDCW90Pt6P5Va6PZ\n\t9YWU40H5IHnrg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=C+gSznDeLeOhjHE6tRT8k8/KIlWt3cH5UYMjPNTNAr8=;\n\tb=zscBpUG3E/rl9NNy49FSxWuY0qYRho7Aq9oG52RsHW3FYBWm/9dp2Y4nF9oIEWHjY/\n\tGtrhsdKOMOermAHQXDtjAJLGLp0Xb0tvdADBPZqzrJ7JgYDGVI6FfxIGW6NtJgiVVceq\n\t7KOGOmdXOAaX+rnBfIHtXsVKBBYAWmDYVPkyiYNPp9VmGzlC1mt4OCLAgMXCa+7tQTxX\n\tdSAwatvcoNkhAmXZy+7EYID0umgqfMkiR/d8Hlxgu38jy84O/BBMy2Kv2+cCZpuzcDDR\n\ttAyIrnQoPlIAircjZ8+QrGHRbzT9wJP/qGVo5Mm79MaWQZCQqF7dB7sIaKiVBL/qzcQn\n\tAY5g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20210112.gappssmtp.com\n\theader.i=@baylibre-com.20210112.gappssmtp.com header.b=\"zscBpUG3\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=C+gSznDeLeOhjHE6tRT8k8/KIlWt3cH5UYMjPNTNAr8=;\n\tb=W9HLv9nKouNwv7D3phy8KMaM9e6wy73OdyZTWMNUarQu6+p/J5yp8bSu0Dlac6OYgG\n\tb8tvoCkR2MvbBbRZbkzOBxbxGSHyehOVhqpo9M1ChL5TdPGNZWyMpKl7GNO65VBIfCuG\n\txWbEqTF4CNf7FPmMjyqvHX7uaWnv8Rog47Pugf8TlstJjapd92w7GUefrWZ5z6I7wxXy\n\tL2YV1F5wtCfPcO198Nl40Qq4ZLoEZYEjrH3Hu7namElH2G4FgEVusqQXMirp/MMXS8uw\n\tAfGGuW/4IVeEjzhIHrNkMYwsk/c1pOqZPQvcsh607XCmfOq9R1UvRPmytCDwYl9H/vIr\n\t9fvw==","X-Gm-Message-State":"ACgBeo3FPIjypu5fWjmICdxky0MOH73EqJL6LLMFFM0N0r1EhKgiPLA1\n\tm0W6kpg/XK4clXHvAR5gEjSlslpaPCLzOweDWRTs/9hWsSXChBhh","X-Google-Smtp-Source":"AA6agR408ac4GGogO/zh4K/xyXqCRbIc3ogmy7TY8p4Av2EPgh/18YFI/XI7L9ic89ynO+dtSD3ggR6iWOUnW8z7df4=","X-Received":"by 2002:a17:90a:6404:b0:1f1:f935:7878 with SMTP id\n\tg4-20020a17090a640400b001f1f9357878mr5506585pjj.218.1659542531065;\n\tWed, 03 Aug 2022 09:02:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20220802001330.17073-1-laurent.pinchart@ideasonboard.com>\n\t<20220803134557.GF311202@pyrite.rasen.tech>","In-Reply-To":"<20220803134557.GF311202@pyrite.rasen.tech>","Date":"Wed, 3 Aug 2022 18:02:00 +0200","Message-ID":"","To":"paul.elder@ideasonboard.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Move ControlInfoMap\n\tto IPA module","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":"Florian Sylvestre via libcamera-devel\n\t","Reply-To":"Florian Sylvestre ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]