[{"id":21121,"web_url":"https://patchwork.libcamera.org/comment/21121/","msgid":"","date":"2021-11-23T11:14:59","subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote:\n> The ISP can use 25 or 81 cells depending on its revision. Remove the\n> cached value in IPARkISP1 and use IPASessionConfiguration to store it\n> and pass it to AGC.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++-\n> src/ipa/rkisp1/algorithms/agc.h | 2 ++\n> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++\n> src/ipa/rkisp1/ipa_context.h | 4 ++++\n> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++---\n> 5 files changed, 36 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 0433813a..e5358ca3 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -61,7 +61,7 @@ Agc::Agc()\n> * \\param[in] context The shared IPA context\n> * \\param[in] configInfo The IPA configuration data\n> *\n> - * \\return 0\n> + * \\return 0 on success or a negative error code otherwise\n> */\n> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> {\n> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \tcontext.frameContext.agc.gain = minAnalogueGain_;\n> \tcontext.frameContext.agc.exposure = 10ms / lineDuration_;\n> \n> +\tswitch (context.configuration.hw.revision) {\n> +\tcase RKISP1_V10:\n> +\t\tnumCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n> +\t\tbreak;\n\nBlank line. Below too.\n\n> +\tcase RKISP1_V12:\n> +\t\tnumCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(RkISP1Agc, Error)\n> +\t\t\t<< \"Hardware revision \" << context.configuration.hw.revision\n> +\t\t\t<< \" is currently not supported\";\n> +\t\treturn -ENODEV;\n\nWe already fail at init() time if the hardware revision is unknown, so\nI'd skip this.\n\nIf you want to make it more full-proof, you could create an enum for the\nhw.revision field, the compiler will then warn if some values are not\nhandled.\n\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index ac95dea5..3ab3f347 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -45,6 +45,8 @@ private:\n> \tdouble minAnalogueGain_;\n> \tdouble maxAnalogueGain_;\n> \n> +\tuint32_t numCells_;\n> +\n> \tutils::Duration filteredExposure_;\n> \tutils::Duration currentExposure_;\n> };\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 16fc248f..cdb952be 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 {\n> * \\brief Maximum analogue gain supported with the configured sensor\n> */\n> \n> +/**\n> + * \\var IPASessionConfiguration::hw\n> + * \\brief RkISP1 specific hardware information\n\ns/ specific/-specific/\n\n> + *\n> + * \\var IPASessionConfiguration::hw.revision\n> + * \\brief RkISP1 revision reported\n\n * \\brief Hardware revision of the ISP (RKISP1_V*)\n\n> + */\n> +\n> /**\n> * \\var IPAFrameContext::agc\n> * \\brief Context for the Automatic Gain Control algorithm\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 8e756fb1..316036c6 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -21,6 +21,10 @@ struct IPASessionConfiguration {\n> \t\tdouble minAnalogueGain;\n> \t\tdouble maxAnalogueGain;\n> \t} agc;\n> +\n> +\tstruct {\n> +\t\tuint32_t revision;\n> +\t} hw;\n> };\n> \n> struct IPAFrameContext {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 59e868ff..a472d111 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -75,7 +75,7 @@ private:\n> \tuint32_t maxGain_;\n> \n> \t/* revision-specific data */\n> -\tunsigned int hwAeMeanMax_;\n> +\tunsigned int hwRevision_;\n> \tunsigned int hwHistBinNMax_;\n> \tunsigned int hwGammaOutMaxSamples_;\n> \tunsigned int hwHistogramWeightGridsSize_;\n> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> \t/* \\todo Add support for other revisions */\n> \tswitch (hwRevision) {\n> \tcase RKISP1_V10:\n> -\t\thwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n> \t\thwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;\n> \t\thwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;\n> \t\thwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;\n> \t\tbreak;\n> \tcase RKISP1_V12:\n> -\t\thwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n> \t\thwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;\n> \t\thwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;\n> \t\thwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;\n> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> \n> \tLOG(IPARkISP1, Debug) << \"Hardware revision is \" << hwRevision;\n> \n> +\t/* Cache the value to set it in configure. */\n> +\thwRevision_ = hwRevision;\n> +\n> \tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> \tif (!camHelper_) {\n> \t\tLOG(IPARkISP1, Error)\n> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> \t/* Clean context at configuration */\n> \tcontext_ = {};\n> \n> +\t/* Set the hardware revision for the algorithms. */\n> +\tcontext_.configuration.hw.revision = hwRevision_;\n> +\n> \t/*\n> \t * When the AGC computes the new exposure values for a frame, it needs\n> \t * to know the limits for shutter speed and analogue gain.","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 66F6DBF415\n\tfor ;\n\tTue, 23 Nov 2021 11:15:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 759536038A;\n\tTue, 23 Nov 2021 12:15:23 +0100 (CET)","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 3BA9C60228\n\tfor ;\n\tTue, 23 Nov 2021 12:15:22 +0100 (CET)","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 A90DAA1B;\n\tTue, 23 Nov 2021 12:15:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iQOa9JVz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637666121;\n\tbh=qZn9w07fUB/OFV9iUupVQh52ojolzVjN7uujIivOGRU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iQOa9JVzacDaycnOmebACbO9TqTL3cKTuZXf6PkrDXKomFBi28V2KjIcrU8MCIT7t\n\tOCOarApbYq3E2csKynexC7l1ewH8voKGpzulTHujGwOzbMjV+QI19++HMUQ65nPZ24\n\tASxDsuwHfqTptaa2kqNlp3w2vQ3VAc2BwhDMM5fE=","Date":"Tue, 23 Nov 2021 13:14:59 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211123091451.67404-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211123091451.67404-12-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211123091451.67404-12-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","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\" "}},{"id":21132,"web_url":"https://patchwork.libcamera.org/comment/21132/","msgid":"<0f7a83ad-11b7-1629-f1cd-435bbd75ff1a@ideasonboard.com>","date":"2021-11-23T13:53:08","subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 23/11/2021 12:14, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote:\n>> The ISP can use 25 or 81 cells depending on its revision. Remove the\n>> cached value in IPARkISP1 and use IPASessionConfiguration to store it\n>> and pass it to AGC.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++-\n>> src/ipa/rkisp1/algorithms/agc.h | 2 ++\n>> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++\n>> src/ipa/rkisp1/ipa_context.h | 4 ++++\n>> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++---\n>> 5 files changed, 36 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>> index 0433813a..e5358ca3 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>> @@ -61,7 +61,7 @@ Agc::Agc()\n>> * \\param[in] context The shared IPA context\n>> * \\param[in] configInfo The IPA configuration data\n>> *\n>> - * \\return 0\n>> + * \\return 0 on success or a negative error code otherwise\n>> */\n>> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>> {\n>> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>> \tcontext.frameContext.agc.gain = minAnalogueGain_;\n>> \tcontext.frameContext.agc.exposure = 10ms / lineDuration_;\n>> \n>> +\tswitch (context.configuration.hw.revision) {\n>> +\tcase RKISP1_V10:\n>> +\t\tnumCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n>> +\t\tbreak;\n> \n> Blank line. Below too.\n> \n>> +\tcase RKISP1_V12:\n>> +\t\tnumCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tLOG(RkISP1Agc, Error)\n>> +\t\t\t<< \"Hardware revision \" << context.configuration.hw.revision\n>> +\t\t\t<< \" is currently not supported\";\n>> +\t\treturn -ENODEV;\n> \n> We already fail at init() time if the hardware revision is unknown, so\n> I'd skip this.\n> \n> If you want to make it more full-proof, you could create an enum for the\n> hw.revision field, the compiler will then warn if some values are not\n> handled.\n> \n\nI hesitated on this one, and there is already the rkisp1_cif_isp_version \nenum and it sounded redundant. Or, I can change:\n struct {\n- uint32_t revision;\n+ rkisp1_cif_isp_version revision;\n } hw;\n };\n\nAnd make it use the enum. But init is called with an unsigned int. \nShould we change it already, move the revision into IPASettings and make \nit a rkisp1_cif_isp_version type too ?\n\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>> \n>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n>> index ac95dea5..3ab3f347 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.h\n>> +++ b/src/ipa/rkisp1/algorithms/agc.h\n>> @@ -45,6 +45,8 @@ private:\n>> \tdouble minAnalogueGain_;\n>> \tdouble maxAnalogueGain_;\n>> \n>> +\tuint32_t numCells_;\n>> +\n>> \tutils::Duration filteredExposure_;\n>> \tutils::Duration currentExposure_;\n>> };\n>> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n>> index 16fc248f..cdb952be 100644\n>> --- a/src/ipa/rkisp1/ipa_context.cpp\n>> +++ b/src/ipa/rkisp1/ipa_context.cpp\n>> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 {\n>> * \\brief Maximum analogue gain supported with the configured sensor\n>> */\n>> \n>> +/**\n>> + * \\var IPASessionConfiguration::hw\n>> + * \\brief RkISP1 specific hardware information\n> \n> s/ specific/-specific/\n> \n>> + *\n>> + * \\var IPASessionConfiguration::hw.revision\n>> + * \\brief RkISP1 revision reported\n> \n> * \\brief Hardware revision of the ISP (RKISP1_V*)\n> \n>> + */\n>> +\n>> /**\n>> * \\var IPAFrameContext::agc\n>> * \\brief Context for the Automatic Gain Control algorithm\n>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n>> index 8e756fb1..316036c6 100644\n>> --- a/src/ipa/rkisp1/ipa_context.h\n>> +++ b/src/ipa/rkisp1/ipa_context.h\n>> @@ -21,6 +21,10 @@ struct IPASessionConfiguration {\n>> \t\tdouble minAnalogueGain;\n>> \t\tdouble maxAnalogueGain;\n>> \t} agc;\n>> +\n>> +\tstruct {\n>> +\t\tuint32_t revision;\n>> +\t} hw;\n>> };\n>> \n>> struct IPAFrameContext {\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 59e868ff..a472d111 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -75,7 +75,7 @@ private:\n>> \tuint32_t maxGain_;\n>> \n>> \t/* revision-specific data */\n>> -\tunsigned int hwAeMeanMax_;\n>> +\tunsigned int hwRevision_;\n>> \tunsigned int hwHistBinNMax_;\n>> \tunsigned int hwGammaOutMaxSamples_;\n>> \tunsigned int hwHistogramWeightGridsSize_;\n>> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n>> \t/* \\todo Add support for other revisions */\n>> \tswitch (hwRevision) {\n>> \tcase RKISP1_V10:\n>> -\t\thwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n>> \t\thwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;\n>> \t\thwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;\n>> \t\thwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;\n>> \t\tbreak;\n>> \tcase RKISP1_V12:\n>> -\t\thwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n>> \t\thwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;\n>> \t\thwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;\n>> \t\thwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;\n>> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n>> \n>> \tLOG(IPARkISP1, Debug) << \"Hardware revision is \" << hwRevision;\n>> \n>> +\t/* Cache the value to set it in configure. */\n>> +\thwRevision_ = hwRevision;\n>> +\n>> \tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n>> \tif (!camHelper_) {\n>> \t\tLOG(IPARkISP1, Error)\n>> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n>> \t/* Clean context at configuration */\n>> \tcontext_ = {};\n>> \n>> +\t/* Set the hardware revision for the algorithms. */\n>> +\tcontext_.configuration.hw.revision = hwRevision_;\n>> +\n>> \t/*\n>> \t * When the AGC computes the new exposure values for a frame, it needs\n>> \t * to know the limits for shutter speed and analogue gain.\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 ABD6ABF415\n\tfor ;\n\tTue, 23 Nov 2021 13:53:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA5CE60230;\n\tTue, 23 Nov 2021 14:53:11 +0100 (CET)","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 B5FD360121\n\tfor ;\n\tTue, 23 Nov 2021 14:53:10 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:3c3b:9149:b:8aa9] (unknown\n\t[IPv6:2a01:e0a:169:7140:3c3b:9149:b:8aa9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 50BEAA1B;\n\tTue, 23 Nov 2021 14:53:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J3hI5zDq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637675590;\n\tbh=arHpoU8QR5ZOUe2WAtJ5T3HLgvzNTR7ECC+dxqOtkII=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=J3hI5zDqM1h2MVQpm7OYptNyzsgjFcBwhPFGUXUDyl5F1XraTPPGCQUpzcaRo0kC0\n\tglsO0n+ESY0tQ8q0WKg/qjwPrxydVXh8cZc+nrykEIcxAfalzl67U1COMfaFkXLG8F\n\tPtCxVDjg1Ukj/3Ab/wWxG336U2OR4BpixBobggA8=","Message-ID":"<0f7a83ad-11b7-1629-f1cd-435bbd75ff1a@ideasonboard.com>","Date":"Tue, 23 Nov 2021 14:53:08 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20211123091451.67404-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211123091451.67404-12-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","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\" "}},{"id":21133,"web_url":"https://patchwork.libcamera.org/comment/21133/","msgid":"<163767624238.3059017.975180257265122873@Monstersaurus>","date":"2021-11-23T14:04:02","subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-11-23 13:53:08)\n> Hi Laurent,\n> \n> On 23/11/2021 12:14, Laurent Pinchart wrote:\n> > Hi Jean-Michel,\n> > \n> > Thank you for the patch.\n> > \n> > On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote:\n> >> The ISP can use 25 or 81 cells depending on its revision. Remove the\n> >> cached value in IPARkISP1 and use IPASessionConfiguration to store it\n> >> and pass it to AGC.\n> >>\n> >> Signed-off-by: Jean-Michel Hautbois \n> >> ---\n> >> src/ipa/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++-\n> >> src/ipa/rkisp1/algorithms/agc.h | 2 ++\n> >> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++\n> >> src/ipa/rkisp1/ipa_context.h | 4 ++++\n> >> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++---\n> >> 5 files changed, 36 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> >> index 0433813a..e5358ca3 100644\n> >> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> >> @@ -61,7 +61,7 @@ Agc::Agc()\n> >> * \\param[in] context The shared IPA context\n> >> * \\param[in] configInfo The IPA configuration data\n> >> *\n> >> - * \\return 0\n> >> + * \\return 0 on success or a negative error code otherwise\n> >> */\n> >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> >> {\n> >> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> >> context.frameContext.agc.gain = minAnalogueGain_;\n> >> context.frameContext.agc.exposure = 10ms / lineDuration_;\n> >> \n> >> + switch (context.configuration.hw.revision) {\n> >> + case RKISP1_V10:\n> >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n> >> + break;\n> > \n> > Blank line. Below too.\n> > \n> >> + case RKISP1_V12:\n> >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n> >> + break;\n> >> + default:\n> >> + LOG(RkISP1Agc, Error)\n> >> + << \"Hardware revision \" << context.configuration.hw.revision\n> >> + << \" is currently not supported\";\n> >> + return -ENODEV;\n> > \n> > We already fail at init() time if the hardware revision is unknown, so\n> > I'd skip this.\n> > \n> > If you want to make it more full-proof, you could create an enum for the\n> > hw.revision field, the compiler will then warn if some values are not\n> > handled.\n> > \n> \n> I hesitated on this one, and there is already the rkisp1_cif_isp_version \n> enum and it sounded redundant. Or, I can change:\n> struct {\n> - uint32_t revision;\n> + rkisp1_cif_isp_version revision;\n> } hw;\n> };\n> \n> And make it use the enum. But init is called with an unsigned int. \n> Should we change it already, move the revision into IPASettings and make \n> it a rkisp1_cif_isp_version type too ?\n\nIsn't IPASettings 'libcamera generic' currently though?\n\nIf so - we'll need to be able to extend this for pipeline specific\nsettings...\n\n--\nKieran\n\n> \n> >> + }\n> >> +\n> >> return 0;\n> >> }\n> >> \n> >> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> >> index ac95dea5..3ab3f347 100644\n> >> --- a/src/ipa/rkisp1/algorithms/agc.h\n> >> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> >> @@ -45,6 +45,8 @@ private:\n> >> double minAnalogueGain_;\n> >> double maxAnalogueGain_;\n> >> \n> >> + uint32_t numCells_;\n> >> +\n> >> utils::Duration filteredExposure_;\n> >> utils::Duration currentExposure_;\n> >> };\n> >> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> >> index 16fc248f..cdb952be 100644\n> >> --- a/src/ipa/rkisp1/ipa_context.cpp\n> >> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> >> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 {\n> >> * \\brief Maximum analogue gain supported with the configured sensor\n> >> */\n> >> \n> >> +/**\n> >> + * \\var IPASessionConfiguration::hw\n> >> + * \\brief RkISP1 specific hardware information\n> > \n> > s/ specific/-specific/\n> > \n> >> + *\n> >> + * \\var IPASessionConfiguration::hw.revision\n> >> + * \\brief RkISP1 revision reported\n> > \n> > * \\brief Hardware revision of the ISP (RKISP1_V*)\n> > \n> >> + */\n> >> +\n> >> /**\n> >> * \\var IPAFrameContext::agc\n> >> * \\brief Context for the Automatic Gain Control algorithm\n> >> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> >> index 8e756fb1..316036c6 100644\n> >> --- a/src/ipa/rkisp1/ipa_context.h\n> >> +++ b/src/ipa/rkisp1/ipa_context.h\n> >> @@ -21,6 +21,10 @@ struct IPASessionConfiguration {\n> >> double minAnalogueGain;\n> >> double maxAnalogueGain;\n> >> } agc;\n> >> +\n> >> + struct {\n> >> + uint32_t revision;\n> >> + } hw;\n> >> };\n> >> \n> >> struct IPAFrameContext {\n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index 59e868ff..a472d111 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -75,7 +75,7 @@ private:\n> >> uint32_t maxGain_;\n> >> \n> >> /* revision-specific data */\n> >> - unsigned int hwAeMeanMax_;\n> >> + unsigned int hwRevision_;\n> >> unsigned int hwHistBinNMax_;\n> >> unsigned int hwGammaOutMaxSamples_;\n> >> unsigned int hwHistogramWeightGridsSize_;\n> >> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> >> /* \\todo Add support for other revisions */\n> >> switch (hwRevision) {\n> >> case RKISP1_V10:\n> >> - hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n> >> hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;\n> >> hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;\n> >> hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;\n> >> break;\n> >> case RKISP1_V12:\n> >> - hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n> >> hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;\n> >> hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;\n> >> hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;\n> >> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> >> \n> >> LOG(IPARkISP1, Debug) << \"Hardware revision is \" << hwRevision;\n> >> \n> >> + /* Cache the value to set it in configure. */\n> >> + hwRevision_ = hwRevision;\n> >> +\n> >> camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> >> if (!camHelper_) {\n> >> LOG(IPARkISP1, Error)\n> >> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> >> /* Clean context at configuration */\n> >> context_ = {};\n> >> \n> >> + /* Set the hardware revision for the algorithms. */\n> >> + context_.configuration.hw.revision = hwRevision_;\n> >> +\n> >> /*\n> >> * When the AGC computes the new exposure values for a frame, it needs\n> >> * to know the limits for shutter speed and analogue gain.\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 C41ABBDB13\n\tfor ;\n\tTue, 23 Nov 2021 14:04:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2876D60230;\n\tTue, 23 Nov 2021 15:04:06 +0100 (CET)","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 1762E60121\n\tfor ;\n\tTue, 23 Nov 2021 15:04:05 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9EDC5A1B;\n\tTue, 23 Nov 2021 15:04:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"S8FWFhtE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637676244;\n\tbh=PLVoQ2JbBIjV0HIiBw84zWeawX7kS6dwmJfwtQawpjk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=S8FWFhtEO3nzbcuKFV+0dlSyGw10PaSFjrMhDWe7MeNOTyfahatQwAmrCWbfAzQW+\n\tCGw+Cdpi77eeyv9wbMEXwdaUS4LbXK82jXr6Ka4gjhnOSdNZwTnVX+p1hXZpfpifCh\n\thciiI81C2tzXA2ZeFHmNnNphv/a4N3cfZDCjWGrg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<0f7a83ad-11b7-1629-f1cd-435bbd75ff1a@ideasonboard.com>","References":"<20211123091451.67404-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211123091451.67404-12-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<0f7a83ad-11b7-1629-f1cd-435bbd75ff1a@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tLaurent Pinchart ","Date":"Tue, 23 Nov 2021 14:04:02 +0000","Message-ID":"<163767624238.3059017.975180257265122873@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","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\" "}},{"id":21135,"web_url":"https://patchwork.libcamera.org/comment/21135/","msgid":"","date":"2021-11-23T14:10:30","subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Nov 23, 2021 at 02:04:02PM +0000, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-11-23 13:53:08)\n> > On 23/11/2021 12:14, Laurent Pinchart wrote:\n> > > On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote:\n> > >> The ISP can use 25 or 81 cells depending on its revision. Remove the\n> > >> cached value in IPARkISP1 and use IPASessionConfiguration to store it\n> > >> and pass it to AGC.\n> > >>\n> > >> Signed-off-by: Jean-Michel Hautbois \n> > >> ---\n> > >> src/ipa/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++-\n> > >> src/ipa/rkisp1/algorithms/agc.h | 2 ++\n> > >> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++\n> > >> src/ipa/rkisp1/ipa_context.h | 4 ++++\n> > >> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++---\n> > >> 5 files changed, 36 insertions(+), 4 deletions(-)\n> > >>\n> > >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > >> index 0433813a..e5358ca3 100644\n> > >> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > >> @@ -61,7 +61,7 @@ Agc::Agc()\n> > >> * \\param[in] context The shared IPA context\n> > >> * \\param[in] configInfo The IPA configuration data\n> > >> *\n> > >> - * \\return 0\n> > >> + * \\return 0 on success or a negative error code otherwise\n> > >> */\n> > >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > >> {\n> > >> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > >> context.frameContext.agc.gain = minAnalogueGain_;\n> > >> context.frameContext.agc.exposure = 10ms / lineDuration_;\n> > >> \n> > >> + switch (context.configuration.hw.revision) {\n> > >> + case RKISP1_V10:\n> > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n> > >> + break;\n> > > \n> > > Blank line. Below too.\n> > > \n> > >> + case RKISP1_V12:\n> > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n> > >> + break;\n> > >> + default:\n> > >> + LOG(RkISP1Agc, Error)\n> > >> + << \"Hardware revision \" << context.configuration.hw.revision\n> > >> + << \" is currently not supported\";\n> > >> + return -ENODEV;\n> > > \n> > > We already fail at init() time if the hardware revision is unknown, so\n> > > I'd skip this.\n> > > \n> > > If you want to make it more full-proof, you could create an enum for the\n> > > hw.revision field, the compiler will then warn if some values are not\n> > > handled.\n> > \n> > I hesitated on this one, and there is already the rkisp1_cif_isp_version \n> > enum and it sounded redundant. Or, I can change:\n> > struct {\n> > - uint32_t revision;\n> > + rkisp1_cif_isp_version revision;\n> > } hw;\n> > };\n\nGood point, we can use that.\n\n> > And make it use the enum. But init is called with an unsigned int. \n> > Should we change it already, move the revision into IPASettings and make \n> > it a rkisp1_cif_isp_version type too ?\n> \n> Isn't IPASettings 'libcamera generic' currently though?\n> \n> If so - we'll need to be able to extend this for pipeline specific\n> settings...\n\nIt is generic, but a hardware revision field seems common enough to be\nincluded in a common structure, even if not used by all platforms.\n\nIf we move the hw revision to IPASettings, then I'd make it a uint32_t\nthere to match the hwrevision field from the MC API, and cast it to an\nenum in the RkISP1 IPA.\n\n> > >> + }\n> > >> +\n> > >> return 0;\n> > >> }\n> > >> \n> > >> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > >> index ac95dea5..3ab3f347 100644\n> > >> --- a/src/ipa/rkisp1/algorithms/agc.h\n> > >> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > >> @@ -45,6 +45,8 @@ private:\n> > >> double minAnalogueGain_;\n> > >> double maxAnalogueGain_;\n> > >> \n> > >> + uint32_t numCells_;\n> > >> +\n> > >> utils::Duration filteredExposure_;\n> > >> utils::Duration currentExposure_;\n> > >> };\n> > >> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > >> index 16fc248f..cdb952be 100644\n> > >> --- a/src/ipa/rkisp1/ipa_context.cpp\n> > >> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > >> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 {\n> > >> * \\brief Maximum analogue gain supported with the configured sensor\n> > >> */\n> > >> \n> > >> +/**\n> > >> + * \\var IPASessionConfiguration::hw\n> > >> + * \\brief RkISP1 specific hardware information\n> > > \n> > > s/ specific/-specific/\n> > > \n> > >> + *\n> > >> + * \\var IPASessionConfiguration::hw.revision\n> > >> + * \\brief RkISP1 revision reported\n> > > \n> > > * \\brief Hardware revision of the ISP (RKISP1_V*)\n> > > \n> > >> + */\n> > >> +\n> > >> /**\n> > >> * \\var IPAFrameContext::agc\n> > >> * \\brief Context for the Automatic Gain Control algorithm\n> > >> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > >> index 8e756fb1..316036c6 100644\n> > >> --- a/src/ipa/rkisp1/ipa_context.h\n> > >> +++ b/src/ipa/rkisp1/ipa_context.h\n> > >> @@ -21,6 +21,10 @@ struct IPASessionConfiguration {\n> > >> double minAnalogueGain;\n> > >> double maxAnalogueGain;\n> > >> } agc;\n> > >> +\n> > >> + struct {\n> > >> + uint32_t revision;\n> > >> + } hw;\n> > >> };\n> > >> \n> > >> struct IPAFrameContext {\n> > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > >> index 59e868ff..a472d111 100644\n> > >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> > >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > >> @@ -75,7 +75,7 @@ private:\n> > >> uint32_t maxGain_;\n> > >> \n> > >> /* revision-specific data */\n> > >> - unsigned int hwAeMeanMax_;\n> > >> + unsigned int hwRevision_;\n> > >> unsigned int hwHistBinNMax_;\n> > >> unsigned int hwGammaOutMaxSamples_;\n> > >> unsigned int hwHistogramWeightGridsSize_;\n> > >> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> > >> /* \\todo Add support for other revisions */\n> > >> switch (hwRevision) {\n> > >> case RKISP1_V10:\n> > >> - hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n> > >> hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;\n> > >> hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;\n> > >> hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;\n> > >> break;\n> > >> case RKISP1_V12:\n> > >> - hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n> > >> hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;\n> > >> hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;\n> > >> hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;\n> > >> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> > >> \n> > >> LOG(IPARkISP1, Debug) << \"Hardware revision is \" << hwRevision;\n> > >> \n> > >> + /* Cache the value to set it in configure. */\n> > >> + hwRevision_ = hwRevision;\n> > >> +\n> > >> camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> > >> if (!camHelper_) {\n> > >> LOG(IPARkISP1, Error)\n> > >> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > >> /* Clean context at configuration */\n> > >> context_ = {};\n> > >> \n> > >> + /* Set the hardware revision for the algorithms. */\n> > >> + context_.configuration.hw.revision = hwRevision_;\n> > >> +\n> > >> /*\n> > >> * When the AGC computes the new exposure values for a frame, it needs\n> > >> * to know the limits for shutter speed and analogue gain.","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 0354DBDB13\n\tfor ;\n\tTue, 23 Nov 2021 14:10:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D30C60230;\n\tTue, 23 Nov 2021 15:10:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 902F060121\n\tfor ;\n\tTue, 23 Nov 2021 15:10:52 +0100 (CET)","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 0FA86F95;\n\tTue, 23 Nov 2021 15:10:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Wd1yzVZR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637676652;\n\tbh=xlidmCDQlnP+4q/MUjMKpiwSvufCyZJqj+w8LbbQ0TM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wd1yzVZRgGGGjqyUxISIfpyCD+QrWR35PAR+bvNo6wXj2nT7H1dF0VGcg1au3Gi4Q\n\tu07ezTQ8mnXYVy1sJNL+aN+hNY12SdnUtK9djlLsOL396GxNAWiR8HkOqY0aCaZBOM\n\tNUCEF7N3dftXXtDhhERvNp95MWNUmfKBxv+hCRcY=","Date":"Tue, 23 Nov 2021 16:10:30 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211123091451.67404-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211123091451.67404-12-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<0f7a83ad-11b7-1629-f1cd-435bbd75ff1a@ideasonboard.com>\n\t<163767624238.3059017.975180257265122873@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163767624238.3059017.975180257265122873@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","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\" "}},{"id":21139,"web_url":"https://patchwork.libcamera.org/comment/21139/","msgid":"","date":"2021-11-23T14:51:04","subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 23/11/2021 15:10, Laurent Pinchart wrote:\n> On Tue, Nov 23, 2021 at 02:04:02PM +0000, Kieran Bingham wrote:\n>> Quoting Jean-Michel Hautbois (2021-11-23 13:53:08)\n>>> On 23/11/2021 12:14, Laurent Pinchart wrote:\n>>>> On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote:\n>>>>> The ISP can use 25 or 81 cells depending on its revision. Remove the\n>>>>> cached value in IPARkISP1 and use IPASessionConfiguration to store it\n>>>>> and pass it to AGC.\n>>>>>\n>>>>> Signed-off-by: Jean-Michel Hautbois \n>>>>> ---\n>>>>> src/ipa/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++-\n>>>>> src/ipa/rkisp1/algorithms/agc.h | 2 ++\n>>>>> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++\n>>>>> src/ipa/rkisp1/ipa_context.h | 4 ++++\n>>>>> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++---\n>>>>> 5 files changed, 36 insertions(+), 4 deletions(-)\n>>>>>\n>>>>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>>>>> index 0433813a..e5358ca3 100644\n>>>>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>>>>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>>>>> @@ -61,7 +61,7 @@ Agc::Agc()\n>>>>> * \\param[in] context The shared IPA context\n>>>>> * \\param[in] configInfo The IPA configuration data\n>>>>> *\n>>>>> - * \\return 0\n>>>>> + * \\return 0 on success or a negative error code otherwise\n>>>>> */\n>>>>> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>>>>> {\n>>>>> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>>>>> context.frameContext.agc.gain = minAnalogueGain_;\n>>>>> context.frameContext.agc.exposure = 10ms / lineDuration_;\n>>>>> \n>>>>> + switch (context.configuration.hw.revision) {\n>>>>> + case RKISP1_V10:\n>>>>> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n>>>>> + break;\n>>>>\n>>>> Blank line. Below too.\n>>>>\n>>>>> + case RKISP1_V12:\n>>>>> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n>>>>> + break;\n>>>>> + default:\n>>>>> + LOG(RkISP1Agc, Error)\n>>>>> + << \"Hardware revision \" << context.configuration.hw.revision\n>>>>> + << \" is currently not supported\";\n>>>>> + return -ENODEV;\n>>>>\n>>>> We already fail at init() time if the hardware revision is unknown, so\n>>>> I'd skip this.\n>>>>\n>>>> If you want to make it more full-proof, you could create an enum for the\n>>>> hw.revision field, the compiler will then warn if some values are not\n>>>> handled.\n>>>\n>>> I hesitated on this one, and there is already the rkisp1_cif_isp_version\n>>> enum and it sounded redundant. Or, I can change:\n>>> struct {\n>>> - uint32_t revision;\n>>> + rkisp1_cif_isp_version revision;\n>>> } hw;\n>>> };\n> \n> Good point, we can use that.\n> \n>>> And make it use the enum. But init is called with an unsigned int.\n>>> Should we change it already, move the revision into IPASettings and make\n>>> it a rkisp1_cif_isp_version type too ?\n>>\n>> Isn't IPASettings 'libcamera generic' currently though?\n>>\n>> If so - we'll need to be able to extend this for pipeline specific\n>> settings...\n> \n> It is generic, but a hardware revision field seems common enough to be\n> included in a common structure, even if not used by all platforms.\n> \n> If we move the hw revision to IPASettings, then I'd make it a uint32_t\n> there to match the hwrevision field from the MC API, and cast it to an\n> enum in the RkISP1 IPA.\n> \n\nAll right, then let's split the difference. I will use the existing enum \nin IPAContext and we'll add a revision field later in IPASettings.\n\n>>>>> + }\n>>>>> +\n>>>>> return 0;\n>>>>> }\n>>>>> \n>>>>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n>>>>> index ac95dea5..3ab3f347 100644\n>>>>> --- a/src/ipa/rkisp1/algorithms/agc.h\n>>>>> +++ b/src/ipa/rkisp1/algorithms/agc.h\n>>>>> @@ -45,6 +45,8 @@ private:\n>>>>> double minAnalogueGain_;\n>>>>> double maxAnalogueGain_;\n>>>>> \n>>>>> + uint32_t numCells_;\n>>>>> +\n>>>>> utils::Duration filteredExposure_;\n>>>>> utils::Duration currentExposure_;\n>>>>> };\n>>>>> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n>>>>> index 16fc248f..cdb952be 100644\n>>>>> --- a/src/ipa/rkisp1/ipa_context.cpp\n>>>>> +++ b/src/ipa/rkisp1/ipa_context.cpp\n>>>>> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 {\n>>>>> * \\brief Maximum analogue gain supported with the configured sensor\n>>>>> */\n>>>>> \n>>>>> +/**\n>>>>> + * \\var IPASessionConfiguration::hw\n>>>>> + * \\brief RkISP1 specific hardware information\n>>>>\n>>>> s/ specific/-specific/\n>>>>\n>>>>> + *\n>>>>> + * \\var IPASessionConfiguration::hw.revision\n>>>>> + * \\brief RkISP1 revision reported\n>>>>\n>>>> * \\brief Hardware revision of the ISP (RKISP1_V*)\n>>>>\n>>>>> + */\n>>>>> +\n>>>>> /**\n>>>>> * \\var IPAFrameContext::agc\n>>>>> * \\brief Context for the Automatic Gain Control algorithm\n>>>>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n>>>>> index 8e756fb1..316036c6 100644\n>>>>> --- a/src/ipa/rkisp1/ipa_context.h\n>>>>> +++ b/src/ipa/rkisp1/ipa_context.h\n>>>>> @@ -21,6 +21,10 @@ struct IPASessionConfiguration {\n>>>>> double minAnalogueGain;\n>>>>> double maxAnalogueGain;\n>>>>> } agc;\n>>>>> +\n>>>>> + struct {\n>>>>> + uint32_t revision;\n>>>>> + } hw;\n>>>>> };\n>>>>> \n>>>>> struct IPAFrameContext {\n>>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>>>> index 59e868ff..a472d111 100644\n>>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>>>> @@ -75,7 +75,7 @@ private:\n>>>>> uint32_t maxGain_;\n>>>>> \n>>>>> /* revision-specific data */\n>>>>> - unsigned int hwAeMeanMax_;\n>>>>> + unsigned int hwRevision_;\n>>>>> unsigned int hwHistBinNMax_;\n>>>>> unsigned int hwGammaOutMaxSamples_;\n>>>>> unsigned int hwHistogramWeightGridsSize_;\n>>>>> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n>>>>> /* \\todo Add support for other revisions */\n>>>>> switch (hwRevision) {\n>>>>> case RKISP1_V10:\n>>>>> - hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n>>>>> hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;\n>>>>> hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;\n>>>>> hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;\n>>>>> break;\n>>>>> case RKISP1_V12:\n>>>>> - hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n>>>>> hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;\n>>>>> hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;\n>>>>> hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;\n>>>>> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n>>>>> \n>>>>> LOG(IPARkISP1, Debug) << \"Hardware revision is \" << hwRevision;\n>>>>> \n>>>>> + /* Cache the value to set it in configure. */\n>>>>> + hwRevision_ = hwRevision;\n>>>>> +\n>>>>> camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n>>>>> if (!camHelper_) {\n>>>>> LOG(IPARkISP1, Error)\n>>>>> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n>>>>> /* Clean context at configuration */\n>>>>> context_ = {};\n>>>>> \n>>>>> + /* Set the hardware revision for the algorithms. */\n>>>>> + context_.configuration.hw.revision = hwRevision_;\n>>>>> +\n>>>>> /*\n>>>>> * When the AGC computes the new exposure values for a frame, it needs\n>>>>> * to know the limits for shutter speed and analogue gain.\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 48C65BDB13\n\tfor ;\n\tTue, 23 Nov 2021 14:51:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81BCF60230;\n\tTue, 23 Nov 2021 15:51:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4FE1660121\n\tfor ;\n\tTue, 23 Nov 2021 15:51:08 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:3c3b:9149:b:8aa9] (unknown\n\t[IPv6:2a01:e0a:169:7140:3c3b:9149:b:8aa9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E47C0993;\n\tTue, 23 Nov 2021 15:51:07 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"blhfEAhZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637679068;\n\tbh=5fu5kT46zRIQxPcqy6ELN6qOGzGfShRkFx4E2cubCiI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=blhfEAhZdFQxBnd2KHQ2ZfCwfnilPeKfSnHW1A9ovVFfgJhAiq1tLWdHTIvteubEQ\n\tb2n4raT/iw/XTm41PayBE5HeGQIUyWrTZxQ47zjOe71YUHJ5LpcRbqvNrGO9Ao5yyg\n\txNGJjTTKsfk3W8dqjHI8IG7CYbt79r2xU8EGDHWo=","Message-ID":"","Date":"Tue, 23 Nov 2021 15:51:04 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20211123091451.67404-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211123091451.67404-12-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<0f7a83ad-11b7-1629-f1cd-435bbd75ff1a@ideasonboard.com>\n\t<163767624238.3059017.975180257265122873@Monstersaurus>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure\n\tthe number of cells used by HW","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\" "}}]