[{"id":31244,"web_url":"https://patchwork.libcamera.org/comment/31244/","msgid":"<172649447644.3421941.12510351942893639646@ping.linuxembedded.co.uk>","date":"2024-09-16T13:47:56","subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-09-06 07:34:43)\n> Currently the rkisp1 pipeline handler only registers controls that are\n> related to the IPA. This patch prepares the rkisp1 pipeline-handler to\n> register camera controls which are not related to the IPA.\n> \n> Hence, introduce an additional ControlInfoMap for IPA controls. These\n> controls will be merged together with the controls in the pipeline\n> handler (introduced subsequently) as part of updateControls() and\n> together will be registered during the registration of the camera.\n> This is similar to what IPU3 pipeline handler handles its controls.\n> \n> Signed-off-by: Umang Jain \n> Reviewed-by: Paul Elder \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++++--\n> 1 file changed, 39 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c02c7cf3..651258e3 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -108,6 +108,8 @@ public:\n> \n> std::unique_ptr ipa_;\n> \n> + ControlInfoMap ipaControls_;\n> +\n> private:\n> void paramFilled(unsigned int frame, unsigned int bytesused);\n> void setSensorControls(unsigned int frame,\n> @@ -183,6 +185,8 @@ private:\n> int allocateBuffers(Camera *camera);\n> int freeBuffers(Camera *camera);\n> \n> + int updateControls(RkISP1CameraData *data);\n> +\n> MediaDevice *media_;\n> std::unique_ptr isp_;\n> std::unique_ptr param_;\n> @@ -364,7 +368,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> }\n> \n> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> - sensorInfo, sensor_->controls(), &controlInfo_);\n> + sensorInfo, sensor_->controls(), &ipaControls_);\n> if (ret < 0) {\n> LOG(RkISP1, Error) << \"IPA initialization failure\";\n> return ret;\n> @@ -814,12 +818,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> ipaConfig.sensorControls = data->sensor_->controls();\n> ipaConfig.paramFormat = paramFormat.fourcc;\n> \n> - ret = data->ipa_->configure(ipaConfig, streamConfig, &data->controlInfo_);\n> + ret = data->ipa_->configure(ipaConfig, streamConfig, &data->ipaControls_);\n> if (ret) {\n> LOG(RkISP1, Error) << \"failed configuring IPA (\" << ret << \")\";\n> return ret;\n> }\n> - return 0;\n> +\n> + return updateControls(data);\n> }\n> \n> int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> @@ -1086,6 +1091,35 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> return 0;\n> }\n> \n> +/**\n> + * \\brief Update the camera controls\n> + * \\param[in] data The camera data\n> + *\n> + * Compute the camera controls by calculating controls which the pipeline\n> + * is reponsible for and merge them with the controls computed by the IPA.\n> + *\n> + * This function needs data->ipaControls_ to be refreshed when a new\n> + * configuration is applied to the camera by the IPA configure() function.\n> + *\n> + * Always call this function after IPA configure() to make sure to have a\n> + * properly refreshed IPA controls list.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> +{\n> + ControlInfoMap::Map controls;\n> +\n> + /* Add the IPA registered controls to list of camera controls. */\n> + for (const auto &ipaControl : data->ipaControls_)\n> + controls[ipaControl.first] = ipaControl.second;\n> +\n\nI guess the merge helpers are on the list - not the info map.\n\nAnd I'll presume we'll see additions for the RkISP1 Pipeline handler\ncontrols get registered in here somehow, so I'll take a look at the next\npatch.\n\nOk - so we indeed need a hook point.\n\n\nReviewed-by: Kieran Bingham \n\n\n\n> + data->controlInfo_ = ControlInfoMap(std::move(controls),\n> + controls::controls);\n> +\n> + return 0;\n> +}\n> +\n> int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> {\n> int ret;\n> @@ -1122,6 +1156,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> if (ret)\n> return ret;\n> \n> + updateControls(data.get());\n> +\n> std::set streams{\n> &data->mainPathStream_,\n> &data->selfPathStream_,\n> -- \n> 2.45.0\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 CF8FCC324C\n\tfor ;\n\tMon, 16 Sep 2024 13:48:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 85A1A634FF;\n\tMon, 16 Sep 2024 15:48:00 +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 53C83634F8\n\tfor ;\n\tMon, 16 Sep 2024 15:47:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 18490480;\n\tMon, 16 Sep 2024 15:46:38 +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=\"kIfIE9cy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726494398;\n\tbh=lSm+LULo8DvuGKAmfwP9+wb+2VZKK3pBow8Hs1daCbE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=kIfIE9cyqEU2a+TVc1WeePXfdGkpcumY5PYkJLT4aEMyNOJjjx2ON0m8RYMNAhCoy\n\tKGdxTahadADRs3gbikSA9lDch08fbvTRlHDW+q6ANn8STlDorsaGQyEfDoo1F78cM8\n\tZzVrXcl7P58B5xpsty+7O8T4jcownwNctzPFBNcc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240906063444.32772-4-umang.jain@ideasonboard.com>","References":"<20240906063444.32772-1-umang.jain@ideasonboard.com>\n\t<20240906063444.32772-4-umang.jain@ideasonboard.com>","Subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","From":"Kieran Bingham ","Cc":"Umang Jain ,\n\tPaul Elder ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 16 Sep 2024 14:47:56 +0100","Message-ID":"<172649447644.3421941.12510351942893639646@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31344,"web_url":"https://patchwork.libcamera.org/comment/31344/","msgid":"<2hepwn4cigq656pb2k4sbjdiibpva3v5zepbvu44hk4otw4h3i@jcoq2c4z2pl3>","date":"2024-09-24T20:18:14","subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Sep 06, 2024 at 12:04:43PM +0530, Umang Jain wrote:\n> Currently the rkisp1 pipeline handler only registers controls that are\n> related to the IPA. This patch prepares the rkisp1 pipeline-handler to\n> register camera controls which are not related to the IPA.\n> \n> Hence, introduce an additional ControlInfoMap for IPA controls. These\n> controls will be merged together with the controls in the pipeline\n> handler (introduced subsequently) as part of updateControls() and\n> together will be registered during the registration of the camera.\n> This is similar to what IPU3 pipeline handler handles its controls.\n> \n> Signed-off-by: Umang Jain \n> Reviewed-by: Paul Elder \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++++--\n> 1 file changed, 39 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c02c7cf3..651258e3 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -108,6 +108,8 @@ public:\n> \n> \tstd::unique_ptr ipa_;\n> \n> +\tControlInfoMap ipaControls_;\n> +\n> private:\n> \tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n> \tvoid setSensorControls(unsigned int frame,\n> @@ -183,6 +185,8 @@ private:\n> \tint allocateBuffers(Camera *camera);\n> \tint freeBuffers(Camera *camera);\n> \n> +\tint updateControls(RkISP1CameraData *data);\n> +\n> \tMediaDevice *media_;\n> \tstd::unique_ptr isp_;\n> \tstd::unique_ptr param_;\n> @@ -364,7 +368,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \t}\n> \n> \tret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> -\t\t\t sensorInfo, sensor_->controls(), &controlInfo_);\n> +\t\t\t sensorInfo, sensor_->controls(), &ipaControls_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> @@ -814,12 +818,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tipaConfig.sensorControls = data->sensor_->controls();\n> \tipaConfig.paramFormat = paramFormat.fourcc;\n> \n> -\tret = data->ipa_->configure(ipaConfig, streamConfig, &data->controlInfo_);\n> +\tret = data->ipa_->configure(ipaConfig, streamConfig, &data->ipaControls_);\n> \tif (ret) {\n> \t\tLOG(RkISP1, Error) << \"failed configuring IPA (\" << ret << \")\";\n> \t\treturn ret;\n> \t}\n> -\treturn 0;\n> +\n> +\treturn updateControls(data);\n> }\n> \n> int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> @@ -1086,6 +1091,35 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Update the camera controls\n> + * \\param[in] data The camera data\n> + *\n> + * Compute the camera controls by calculating controls which the pipeline\n> + * is reponsible for and merge them with the controls computed by the IPA.\n> + *\n> + * This function needs data->ipaControls_ to be refreshed when a new\n> + * configuration is applied to the camera by the IPA configure() function.\n> + *\n> + * Always call this function after IPA configure() to make sure to have a\n> + * properly refreshed IPA controls list.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> +{\n> +\tControlInfoMap::Map controls;\n> +\n> +\t/* Add the IPA registered controls to list of camera controls. */\n> +\tfor (const auto &ipaControl : data->ipaControls_)\n> +\t\tcontrols[ipaControl.first] = ipaControl.second;\n\nI guess I'm missing something here. Why is that extra ipaControls_\nneeded? At this point in time the controlInfo_ should be populated/reset\nby the ipa and we could just add the extra controls to it here.\n\nBest regards,\nStefan\n\n> +\n> +\tdata->controlInfo_ = ControlInfoMap(std::move(controls),\n> +\t\t\t\t\t controls::controls);\n> +\n> +\treturn 0;\n> +}\n> +\n> int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> {\n> \tint ret;\n> @@ -1122,6 +1156,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tupdateControls(data.get());\n> +\n> \tstd::set streams{\n> \t\t&data->mainPathStream_,\n> \t\t&data->selfPathStream_,\n> -- \n> 2.45.0\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 0F92DC0F1B\n\tfor ;\n\tTue, 24 Sep 2024 20:18:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 04B7E6350F;\n\tTue, 24 Sep 2024 22:18:19 +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 A6DF663500\n\tfor ;\n\tTue, 24 Sep 2024 22:18:17 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:f788:1cdc:64c2:c2c9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7474FC8E;\n\tTue, 24 Sep 2024 22:16: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=\"Y2blIrbN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727209010;\n\tbh=v3x50d4kMH8OSvEZ3V+/BIprRlPty4h5QziBP4fRCgg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y2blIrbNNvaxIWW+y4coqEPGophzD+dTd57NX7h9Z1XHIcuOR1fpE22NSAZznKyVK\n\tqifCgf6+R7nEzHJHcv8l+pl5evJkO+Iwj530/JEcNXRBEs2IbTW6WmbA2vhCBsRQ7N\n\tnwBISrhqM65iRwSVX7EVHj/Fagy0K7yBNppX1J+c=","Date":"Tue, 24 Sep 2024 22:18:14 +0200","From":"Stefan Klug ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","Message-ID":"<2hepwn4cigq656pb2k4sbjdiibpva3v5zepbvu44hk4otw4h3i@jcoq2c4z2pl3>","References":"<20240906063444.32772-1-umang.jain@ideasonboard.com>\n\t<20240906063444.32772-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240906063444.32772-4-umang.jain@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31346,"web_url":"https://patchwork.libcamera.org/comment/31346/","msgid":"<0ef39fe2-6ab4-4cca-b013-9978e91af963@ideasonboard.com>","date":"2024-09-25T06:23:09","subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Stefan\n\nOn 25/09/24 1:48 am, Stefan Klug wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 06, 2024 at 12:04:43PM +0530, Umang Jain wrote:\n>> Currently the rkisp1 pipeline handler only registers controls that are\n>> related to the IPA. This patch prepares the rkisp1 pipeline-handler to\n>> register camera controls which are not related to the IPA.\n>>\n>> Hence, introduce an additional ControlInfoMap for IPA controls. These\n>> controls will be merged together with the controls in the pipeline\n>> handler (introduced subsequently) as part of updateControls() and\n>> together will be registered during the registration of the camera.\n>> This is similar to what IPU3 pipeline handler handles its controls.\n>>\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Paul Elder \n>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++++--\n>> 1 file changed, 39 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index c02c7cf3..651258e3 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -108,6 +108,8 @@ public:\n>> \n>> \tstd::unique_ptr ipa_;\n>> \n>> +\tControlInfoMap ipaControls_;\n>> +\n>> private:\n>> \tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n>> \tvoid setSensorControls(unsigned int frame,\n>> @@ -183,6 +185,8 @@ private:\n>> \tint allocateBuffers(Camera *camera);\n>> \tint freeBuffers(Camera *camera);\n>> \n>> +\tint updateControls(RkISP1CameraData *data);\n>> +\n>> \tMediaDevice *media_;\n>> \tstd::unique_ptr isp_;\n>> \tstd::unique_ptr param_;\n>> @@ -364,7 +368,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n>> \t}\n>> \n>> \tret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n>> -\t\t\t sensorInfo, sensor_->controls(), &controlInfo_);\n>> +\t\t\t sensorInfo, sensor_->controls(), &ipaControls_);\n>> \tif (ret < 0) {\n>> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n>> \t\treturn ret;\n>> @@ -814,12 +818,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>> \tipaConfig.sensorControls = data->sensor_->controls();\n>> \tipaConfig.paramFormat = paramFormat.fourcc;\n>> \n>> -\tret = data->ipa_->configure(ipaConfig, streamConfig, &data->controlInfo_);\n>> +\tret = data->ipa_->configure(ipaConfig, streamConfig, &data->ipaControls_);\n>> \tif (ret) {\n>> \t\tLOG(RkISP1, Error) << \"failed configuring IPA (\" << ret << \")\";\n>> \t\treturn ret;\n>> \t}\n>> -\treturn 0;\n>> +\n>> +\treturn updateControls(data);\n>> }\n>> \n>> int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n>> @@ -1086,6 +1091,35 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n>> \treturn 0;\n>> }\n>> \n>> +/**\n>> + * \\brief Update the camera controls\n>> + * \\param[in] data The camera data\n>> + *\n>> + * Compute the camera controls by calculating controls which the pipeline\n>> + * is reponsible for and merge them with the controls computed by the IPA.\n>> + *\n>> + * This function needs data->ipaControls_ to be refreshed when a new\n>> + * configuration is applied to the camera by the IPA configure() function.\n>> + *\n>> + * Always call this function after IPA configure() to make sure to have a\n>> + * properly refreshed IPA controls list.\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n>> +{\n>> +\tControlInfoMap::Map controls;\n>> +\n>> +\t/* Add the IPA registered controls to list of camera controls. */\n>> +\tfor (const auto &ipaControl : data->ipaControls_)\n>> +\t\tcontrols[ipaControl.first] = ipaControl.second;\n> I guess I'm missing something here. Why is that extra ipaControls_\n> needed? At this point in time the controlInfo_ should be populated/reset\n> by the ipa and we could just add the extra controls to it here.\n\nOne cannot add controls to the ControlInfoMap directly. It  has be added \nto ControlInfoMap::Map first and then a ControlInfoMap has to be \nconstructed out of it.\n\nIf you read the ControlInfoMap class definition, it has a private \ninheritance from std::unordered_map<>, hence all accessors/modifiers are \nprivate in ControlInfoMap (for .e.g insert, emplace etc.)\n\nDoes this makes sense?\n> Best regards,\n> Stefan\n>\n>> +\n>> +\tdata->controlInfo_ = ControlInfoMap(std::move(controls),\n>> +\t\t\t\t\t controls::controls);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>> {\n>> \tint ret;\n>> @@ -1122,6 +1156,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>> \tif (ret)\n>> \t\treturn ret;\n>> \n>> +\tupdateControls(data.get());\n>> +\n>> \tstd::set streams{\n>> \t\t&data->mainPathStream_,\n>> \t\t&data->selfPathStream_,\n>> -- \n>> 2.45.0\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 D6D24C0F1B\n\tfor ;\n\tWed, 25 Sep 2024 06:23:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98A1E6350F;\n\tWed, 25 Sep 2024 08:23:17 +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 669C2618D8\n\tfor ;\n\tWed, 25 Sep 2024 08:23:15 +0200 (CEST)","from [IPV6:2405:201:2015:f873:c173:4b:4a04:3a21] (unknown\n\t[IPv6:2405:201:2015:f873:c173:4b:4a04:3a21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 64A195B2;\n\tWed, 25 Sep 2024 08:21:47 +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=\"DqgwVctf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727245308;\n\tbh=ozTZmj9QqARr/WB9pa12kqWl2S70fRlznQtERjRKQ5k=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=DqgwVctfVPkbUKz/+KsFNrz3lP4dp62PEBhK8MQY6s/qpCvbvvgq5Qc04qvyfdxmT\n\tzdMGH/OFHiqI05//db9VOPTtu1dhEhpSfWRWz/VobzNg0/gphQaIoVrrRmQDyrIWVq\n\tDUEa0z+sWXgBndkx8pda6hvmpfw+ZC8sY4NQz/SQ=","Message-ID":"<0ef39fe2-6ab4-4cca-b013-9978e91af963@ideasonboard.com>","Date":"Wed, 25 Sep 2024 11:53:09 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","References":"<20240906063444.32772-1-umang.jain@ideasonboard.com>\n\t<20240906063444.32772-4-umang.jain@ideasonboard.com>\n\t<2hepwn4cigq656pb2k4sbjdiibpva3v5zepbvu44hk4otw4h3i@jcoq2c4z2pl3>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<2hepwn4cigq656pb2k4sbjdiibpva3v5zepbvu44hk4otw4h3i@jcoq2c4z2pl3>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31348,"web_url":"https://patchwork.libcamera.org/comment/31348/","msgid":"","date":"2024-09-25T07:00:32","subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Sep 25, 2024 at 11:53:09AM +0530, Umang Jain wrote:\n> Hi Stefan\n> \n> On 25/09/24 1:48 am, Stefan Klug wrote:\n> > Hi Umang,\n> > \n> > Thank you for the patch.\n> > \n> > On Fri, Sep 06, 2024 at 12:04:43PM +0530, Umang Jain wrote:\n> > > Currently the rkisp1 pipeline handler only registers controls that are\n> > > related to the IPA. This patch prepares the rkisp1 pipeline-handler to\n> > > register camera controls which are not related to the IPA.\n> > > \n> > > Hence, introduce an additional ControlInfoMap for IPA controls. These\n> > > controls will be merged together with the controls in the pipeline\n> > > handler (introduced subsequently) as part of updateControls() and\n> > > together will be registered during the registration of the camera.\n> > > This is similar to what IPU3 pipeline handler handles its controls.\n> > > \n> > > Signed-off-by: Umang Jain \n> > > Reviewed-by: Paul Elder \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++++--\n> > > 1 file changed, 39 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index c02c7cf3..651258e3 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -108,6 +108,8 @@ public:\n> > > \tstd::unique_ptr ipa_;\n> > > +\tControlInfoMap ipaControls_;\n> > > +\n> > > private:\n> > > \tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n> > > \tvoid setSensorControls(unsigned int frame,\n> > > @@ -183,6 +185,8 @@ private:\n> > > \tint allocateBuffers(Camera *camera);\n> > > \tint freeBuffers(Camera *camera);\n> > > +\tint updateControls(RkISP1CameraData *data);\n> > > +\n> > > \tMediaDevice *media_;\n> > > \tstd::unique_ptr isp_;\n> > > \tstd::unique_ptr param_;\n> > > @@ -364,7 +368,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > \t}\n> > > \tret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > > -\t\t\t sensorInfo, sensor_->controls(), &controlInfo_);\n> > > +\t\t\t sensorInfo, sensor_->controls(), &ipaControls_);\n> > > \tif (ret < 0) {\n> > > \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> > > \t\treturn ret;\n> > > @@ -814,12 +818,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > \tipaConfig.sensorControls = data->sensor_->controls();\n> > > \tipaConfig.paramFormat = paramFormat.fourcc;\n> > > -\tret = data->ipa_->configure(ipaConfig, streamConfig, &data->controlInfo_);\n> > > +\tret = data->ipa_->configure(ipaConfig, streamConfig, &data->ipaControls_);\n> > > \tif (ret) {\n> > > \t\tLOG(RkISP1, Error) << \"failed configuring IPA (\" << ret << \")\";\n> > > \t\treturn ret;\n> > > \t}\n> > > -\treturn 0;\n> > > +\n> > > +\treturn updateControls(data);\n> > > }\n> > > int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> > > @@ -1086,6 +1091,35 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> > > \treturn 0;\n> > > }\n> > > +/**\n> > > + * \\brief Update the camera controls\n> > > + * \\param[in] data The camera data\n> > > + *\n> > > + * Compute the camera controls by calculating controls which the pipeline\n> > > + * is reponsible for and merge them with the controls computed by the IPA.\n> > > + *\n> > > + * This function needs data->ipaControls_ to be refreshed when a new\n> > > + * configuration is applied to the camera by the IPA configure() function.\n> > > + *\n> > > + * Always call this function after IPA configure() to make sure to have a\n> > > + * properly refreshed IPA controls list.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> > > +{\n> > > +\tControlInfoMap::Map controls;\n> > > +\n> > > +\t/* Add the IPA registered controls to list of camera controls. */\n> > > +\tfor (const auto &ipaControl : data->ipaControls_)\n> > > +\t\tcontrols[ipaControl.first] = ipaControl.second;\n> > I guess I'm missing something here. Why is that extra ipaControls_\n> > needed? At this point in time the controlInfo_ should be populated/reset\n> > by the ipa and we could just add the extra controls to it here.\n> \n> One cannot add controls to the ControlInfoMap directly. It  has be added to\n> ControlInfoMap::Map first and then a ControlInfoMap has to be constructed\n> out of it.\n> \n> If you read the ControlInfoMap class definition, it has a private\n> inheritance from std::unordered_map<>, hence all accessors/modifiers are\n> private in ControlInfoMap (for .e.g insert, emplace etc.)\n> \n> Does this makes sense?\n\nOh yes, that makes a lot of sense. I somehow missed the private. Thanks\nfor clarification. Makes me think if we should question this\nimmutability. But that's for another day.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> > Best regards,\n> > Stefan\n> > \n> > > +\n> > > +\tdata->controlInfo_ = ControlInfoMap(std::move(controls),\n> > > +\t\t\t\t\t controls::controls);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > {\n> > > \tint ret;\n> > > @@ -1122,6 +1156,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > +\tupdateControls(data.get());\n> > > +\n> > > \tstd::set streams{\n> > > \t\t&data->mainPathStream_,\n> > > \t\t&data->selfPathStream_,\n> > > -- \n> > > 2.45.0\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 9967DC0F1B\n\tfor ;\n\tWed, 25 Sep 2024 07:00:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 608636350F;\n\tWed, 25 Sep 2024 09:00:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B042634F4\n\tfor ;\n\tWed, 25 Sep 2024 09:00:36 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:f788:1cdc:64c2:c2c9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFBB85B2;\n\tWed, 25 Sep 2024 08:59:08 +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=\"XLJ0bZdc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727247548;\n\tbh=p9fteGdu8/aPuzwgSNPuhKCzB3Y4Q1ix9B4HEuGbLI8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XLJ0bZdcF+3gSIfKefDMpVgnJOIHvDF+vz5751K+qk/rGTS/rDDBfYS6BfKwNyAFI\n\tU24SyTXjzRBzYKlmb0anulm3QWVGV9L5G5seMZy5mCGKC11dJbrVWhas8m9EfhIryW\n\t1fHKobN5SgVFFD5WkI80GVUJ0pPhiRCc1HnD2+Io=","Date":"Wed, 25 Sep 2024 09:00:32 +0200","From":"Stefan Klug ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","Message-ID":"","References":"<20240906063444.32772-1-umang.jain@ideasonboard.com>\n\t<20240906063444.32772-4-umang.jain@ideasonboard.com>\n\t<2hepwn4cigq656pb2k4sbjdiibpva3v5zepbvu44hk4otw4h3i@jcoq2c4z2pl3>\n\t<0ef39fe2-6ab4-4cca-b013-9978e91af963@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<0ef39fe2-6ab4-4cca-b013-9978e91af963@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31359,"web_url":"https://patchwork.libcamera.org/comment/31359/","msgid":"<20240925182129.GF27666@pendragon.ideasonboard.com>","date":"2024-09-25T18:21:29","subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","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, Sep 06, 2024 at 12:04:43PM +0530, Umang Jain wrote:\n> Currently the rkisp1 pipeline handler only registers controls that are\n> related to the IPA. This patch prepares the rkisp1 pipeline-handler to\n> register camera controls which are not related to the IPA.\n> \n> Hence, introduce an additional ControlInfoMap for IPA controls. These\n> controls will be merged together with the controls in the pipeline\n> handler (introduced subsequently) as part of updateControls() and\n> together will be registered during the registration of the camera.\n> This is similar to what IPU3 pipeline handler handles its controls.\n> \n> Signed-off-by: Umang Jain \n> Reviewed-by: Paul Elder \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++++--\n> 1 file changed, 39 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c02c7cf3..651258e3 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -108,6 +108,8 @@ public:\n> \n> \tstd::unique_ptr ipa_;\n> \n> +\tControlInfoMap ipaControls_;\n> +\n> private:\n> \tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n> \tvoid setSensorControls(unsigned int frame,\n> @@ -183,6 +185,8 @@ private:\n> \tint allocateBuffers(Camera *camera);\n> \tint freeBuffers(Camera *camera);\n> \n> +\tint updateControls(RkISP1CameraData *data);\n> +\n> \tMediaDevice *media_;\n> \tstd::unique_ptr isp_;\n> \tstd::unique_ptr param_;\n> @@ -364,7 +368,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \t}\n> \n> \tret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> -\t\t\t sensorInfo, sensor_->controls(), &controlInfo_);\n> +\t\t\t sensorInfo, sensor_->controls(), &ipaControls_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> @@ -814,12 +818,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tipaConfig.sensorControls = data->sensor_->controls();\n> \tipaConfig.paramFormat = paramFormat.fourcc;\n> \n> -\tret = data->ipa_->configure(ipaConfig, streamConfig, &data->controlInfo_);\n> +\tret = data->ipa_->configure(ipaConfig, streamConfig, &data->ipaControls_);\n> \tif (ret) {\n> \t\tLOG(RkISP1, Error) << \"failed configuring IPA (\" << ret << \")\";\n> \t\treturn ret;\n> \t}\n> -\treturn 0;\n> +\n> +\treturn updateControls(data);\n> }\n> \n> int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> @@ -1086,6 +1091,35 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Update the camera controls\n> + * \\param[in] data The camera data\n> + *\n> + * Compute the camera controls by calculating controls which the pipeline\n> + * is reponsible for and merge them with the controls computed by the IPA.\n> + *\n> + * This function needs data->ipaControls_ to be refreshed when a new\n> + * configuration is applied to the camera by the IPA configure() function.\n> + *\n> + * Always call this function after IPA configure() to make sure to have a\n> + * properly refreshed IPA controls list.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> +{\n> +\tControlInfoMap::Map controls;\n> +\n> +\t/* Add the IPA registered controls to list of camera controls. */\n> +\tfor (const auto &ipaControl : data->ipaControls_)\n> +\t\tcontrols[ipaControl.first] = ipaControl.second;\n> +\n> +\tdata->controlInfo_ = ControlInfoMap(std::move(controls),\n> +\t\t\t\t\t controls::controls);\n> +\n> +\treturn 0;\n> +}\n> +\n> int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> {\n> \tint ret;\n> @@ -1122,6 +1156,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tupdateControls(data.get());\n> +\n> \tstd::set streams{\n> \t\t&data->mainPathStream_,\n> \t\t&data->selfPathStream_,","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 369AFC0F1B\n\tfor ;\n\tWed, 25 Sep 2024 18:21:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D65563510;\n\tWed, 25 Sep 2024 20:21:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11494634FB\n\tfor ;\n\tWed, 25 Sep 2024 20:21:32 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E7DBA30;\n\tWed, 25 Sep 2024 20:20:03 +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=\"JgorR0V8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727288404;\n\tbh=F7MZL2/1uIs0wVrKJ/dSYKjSM2nrtbO+VTahbKzlLMU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JgorR0V8g6PqnzEgRNySOG9hYvW5kC74ZU5vaXQeaLgavNybFKoh9dizGV3pk8RlJ\n\tbjgdpFnzY8aCaGExehAimPUY3axTusxo8oBvb6hXXMW21HZ4gFYuzERN9xeepDwgIA\n\tfQR+g5EYrC8bTSzxsUgGfLKEHhfTHOjOd5iD4Qrg=","Date":"Wed, 25 Sep 2024 21:21:29 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera\n\tcontrols","Message-ID":"<20240925182129.GF27666@pendragon.ideasonboard.com>","References":"<20240906063444.32772-1-umang.jain@ideasonboard.com>\n\t<20240906063444.32772-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240906063444.32772-4-umang.jain@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]