[{"id":28934,"web_url":"https://patchwork.libcamera.org/comment/28934/","msgid":"<87zfv331r0.fsf@redhat.com>","date":"2024-03-12T15:31:47","subject":"Re: [PATCH v5 18/18] libcamera: Soft IPA: use CameraSensorHelper\n\tfor analogue gain","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hans de Goede writes:\n\n> From: Andrei Konovalov \n>\n> Use CameraSensorHelper to convert the analogue gain code read from the\n> camera sensor into real analogue gain value. In the future this makes\n> it possible to use faster AE/AGC algorithm. For now the same AE/AGC\n> algorithm is used, but even then the CameraSensorHelper lets us use the\n> full range of analogue gain values.\n>\n> If there is no CameraSensorHelper for the camera sensor in use, a\n> warning log message is printed, and the AE/AGC works exactly as before\n> this change.\n>\n> Signed-off-by: Andrei Konovalov \n> Reviewed-by: Hans de Goede \n> Signed-off-by: Hans de Goede \n\nReviewed-by: Milan Zamazal \n\n> ---\n> .../internal/software_isp/software_isp.h | 3 +-\n> src/ipa/simple/soft_simple.cpp | 77 ++++++++++++-------\n> src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> src/libcamera/software_isp/software_isp.cpp | 8 +-\n> 4 files changed, 57 insertions(+), 33 deletions(-)\n>\n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index 8d25e979..2a6db7ba 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -26,6 +26,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/dma_heaps.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> #include \"libcamera/internal/shared_mem_object.h\"\n> @@ -43,7 +44,7 @@ LOG_DECLARE_CATEGORY(SoftwareIsp)\n> class SoftwareIsp\n> {\n> public:\n> -\tSoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);\n> +\tSoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor);\n> \t~SoftwareIsp();\n> \n> \tint loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index ac027568..e4d64762 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -22,6 +22,8 @@\n> #include \"libcamera/internal/software_isp/debayer_params.h\"\n> #include \"libcamera/internal/software_isp/swisp_stats.h\"\n> \n> +#include \"libipa/camera_sensor_helper.h\"\n> +\n> #include \"black_level.h\"\n> \n> namespace libcamera {\n> @@ -67,18 +69,27 @@ private:\n> \tDebayerParams *params_;\n> \tSwIspStats *stats_;\n> \tBlackLevel blackLevel_;\n> +\tstd::unique_ptr camHelper_;\n> \n> \tint32_t exposure_min_, exposure_max_;\n> -\tint32_t again_min_, again_max_;\n> -\tint32_t again_, exposure_;\n> +\tint32_t exposure_;\n> +\tdouble again_min_, again_max_, againMinStep_;\n> +\tdouble again_;\n> \tunsigned int ignore_updates_;\n> };\n> \n> -int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,\n> +int IPASoftSimple::init(const IPASettings &settings,\n> \t\t\tconst SharedFD &fdStats,\n> \t\t\tconst SharedFD &fdParams,\n> \t\t\tconst ControlInfoMap &sensorInfoMap)\n> {\n> +\tcamHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> +\tif (camHelper_ == nullptr) {\n> +\t\tLOG(IPASoft, Warning)\n> +\t\t\t<< \"Failed to create camera sensor helper for \"\n> +\t\t\t<< settings.sensorModel;\n> +\t}\n> +\n> \tfdStats_ = fdStats;\n> \tif (!fdStats_.isValid()) {\n> \t\tLOG(IPASoft, Error) << \"Invalid Statistics handle\";\n> @@ -132,25 +143,35 @@ int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n> \t\texposure_min_ = 1;\n> \t}\n> \n> -\tagain_min_ = gain_info.min().get();\n> -\tagain_max_ = gain_info.max().get();\n> -\t/*\n> -\t * The camera sensor gain (g) is usually not equal to the value written\n> -\t * into the gain register (x). But the way how the AGC algorithm changes\n> -\t * the gain value to make the total exposure closer to the optimum assumes\n> -\t * that g(x) is not too far from linear function. If the minimal gain is 0,\n> -\t * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).\n> -\t * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near\n> -\t * one edge, and very small near the other) we limit the range of the gain\n> -\t * values used.\n> -\t */\n> -\tif (!again_min_) {\n> -\t\tLOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n> -\t\tagain_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);\n> +\tint32_t again_min = gain_info.min().get();\n> +\tint32_t again_max = gain_info.max().get();\n> +\n> +\tif (camHelper_) {\n> +\t\tagain_min_ = camHelper_->gain(again_min);\n> +\t\tagain_max_ = camHelper_->gain(again_max);\n> +\t\tagainMinStep_ = (again_max_ - again_min_) / 100.0;\n> +\t} else {\n> +\t\t/*\n> +\t\t * The camera sensor gain (g) is usually not equal to the value written\n> +\t\t * into the gain register (x). But the way how the AGC algorithm changes\n> +\t\t * the gain value to make the total exposure closer to the optimum assumes\n> +\t\t * that g(x) is not too far from linear function. If the minimal gain is 0,\n> +\t\t * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).\n> +\t\t * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near\n> +\t\t * one edge, and very small near the other) we limit the range of the gain\n> +\t\t * values used.\n> +\t\t */\n> +\t\tagain_max_ = again_max;\n> +\t\tif (!again_min) {\n> +\t\t\tLOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n> +\t\t\tagain_min_ = std::min(100, again_min / 2 + again_max / 2);\n> +\t\t}\n> +\t\tagainMinStep_ = 1.0;\n> \t}\n> \n> \tLOG(IPASoft, Info) << \"Exposure \" << exposure_min_ << \"-\" << exposure_max_\n> -\t\t\t << \", gain \" << again_min_ << \"-\" << again_max_;\n> +\t\t\t << \", gain \" << again_min_ << \"-\" << again_max_\n> +\t\t\t << \" (\" << againMinStep_ << \")\";\n> \n> \treturn 0;\n> }\n> @@ -252,12 +273,14 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \tControlList ctrls(sensorControls);\n> \n> \texposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n> -\tagain_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\tint32_t again = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\tagain_ = camHelper_ ? camHelper_->gain(again) : again;\n> \n> \tupdateExposure(exposureMSV);\n> \n> \tctrls.set(V4L2_CID_EXPOSURE, exposure_);\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);\n> +\tctrls.set(V4L2_CID_ANALOGUE_GAIN,\n> +\t\t static_cast(camHelper_ ? camHelper_->gainCode(again_) : again_));\n> \n> \tignore_updates_ = 2;\n> \n> @@ -276,7 +299,7 @@ void IPASoftSimple::updateExposure(double exposureMSV)\n> \tstatic constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;\n> \tstatic constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;\n> \n> -\tint next;\n> +\tdouble next;\n> \n> \tif (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n> \t\tnext = exposure_ * kExpNumeratorUp / kExpDenominator;\n> @@ -286,18 +309,18 @@ void IPASoftSimple::updateExposure(double exposureMSV)\n> \t\t\texposure_ = next;\n> \t\tif (exposure_ >= exposure_max_) {\n> \t\t\tnext = again_ * kExpNumeratorUp / kExpDenominator;\n> -\t\t\tif (next - again_ < 1)\n> -\t\t\t\tagain_ += 1;\n> +\t\t\tif (next - again_ < againMinStep_)\n> +\t\t\t\tagain_ += againMinStep_;\n> \t\t\telse\n> \t\t\t\tagain_ = next;\n> \t\t}\n> \t}\n> \n> \tif (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n> -\t\tif (exposure_ == exposure_max_ && again_ != again_min_) {\n> +\t\tif (exposure_ == exposure_max_ && again_ > again_min_) {\n> \t\t\tnext = again_ * kExpNumeratorDown / kExpDenominator;\n> -\t\t\tif (again_ - next < 1)\n> -\t\t\t\tagain_ -= 1;\n> +\t\t\tif (again_ - next < againMinStep_)\n> +\t\t\t\tagain_ -= againMinStep_;\n> \t\t\telse\n> \t\t\t\tagain_ = next;\n> \t\t} else {\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index e491ab62..32b56c57 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -526,7 +526,7 @@ int SimpleCameraData::init()\n> \t * Instantiate Soft ISP if this is enabled for the given driver and no converter is used.\n> \t */\n> \tif (!converter_ && pipe->swIspEnabled()) {\n> -\t\tswIsp_ = std::make_unique(pipe, sensor_->controls());\n> +\t\tswIsp_ = std::make_unique(pipe, sensor_.get());\n> \t\tif (!swIsp_->isValid()) {\n> \t\t\tLOG(SimplePipeline, Warning)\n> \t\t\t\t<< \"Failed to create software ISP, disabling software debayering\";\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index 9b49be41..ea4d96e4 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -60,9 +60,9 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> /**\n> * \\brief Constructs SoftwareIsp object\n> * \\param[in] pipe The pipeline handler in use\n> - * \\param[in] sensorControls ControlInfoMap describing the controls supported by the sensor\n> + * \\param[in] sensor Pointer to the CameraSensor instance owned by the pipeline handler\n> */\n> -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls)\n> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> \t: debayer_(nullptr),\n> \t debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f, 0 },\n> \t dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> @@ -97,10 +97,10 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorCont\n> \t\treturn;\n> \t}\n> \n> -\tint ret = ipa_->init(IPASettings{ \"No cfg file\", \"No sensor model\" },\n> +\tint ret = ipa_->init(IPASettings{ \"No cfg file\", sensor->model() },\n> \t\t\t debayer_->getStatsFD(),\n> \t\t\t sharedParams_.fd(),\n> -\t\t\t sensorControls);\n> +\t\t\t sensor->controls());\n> \tif (ret) {\n> \t\tLOG(SoftwareIsp, Error) << \"IPA init failed\";\n> \t\tdebayer_.reset();","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 034F8BD1F1\n\tfor ;\n\tTue, 12 Mar 2024 15:31:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AD2E6286C;\n\tTue, 12 Mar 2024 16:31:56 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 220CE6286C\n\tfor ;\n\tTue, 12 Mar 2024 16:31:55 +0100 (CET)","from mail-lf1-f71.google.com (mail-lf1-f71.google.com\n\t[209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-68-GE_XQ3jCNa-0fbrscUE-Nw-1; Tue, 12 Mar 2024 11:31:50 -0400","by mail-lf1-f71.google.com with SMTP id\n\t2adb3069b0e04-5139c92ba2eso3009546e87.1\n\tfor ;\n\tTue, 12 Mar 2024 08:31:50 -0700 (PDT)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\tn2-20020a5d51c2000000b0033e192a5852sm9277915wrv.30.2024.03.12.08.31.47\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 12 Mar 2024 08:31:47 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"jEejblVC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1710257514;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=SZ2paO7UOAp+uvOOenh5X5Cz2VdaPFOK9H08IsPGG3k=;\n\tb=jEejblVC+c1VV7v3oMlOuIyYcZeCVQXaanSuorLFOWlaNk9G4KGbLOR89bMS0LQ4YhiIA5\n\t0zVv/0Yyl5AwPNOCk3LgaLMrZ2BfDTKHEagfpqj0biNZ20qShmctp/4ObvjEne+v9MMfqU\n\toRDsEqCrL7Ogh/3+rhjzIyVG/eFx4qA=","X-MC-Unique":"GE_XQ3jCNa-0fbrscUE-Nw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1710257509; x=1710862309;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=SZ2paO7UOAp+uvOOenh5X5Cz2VdaPFOK9H08IsPGG3k=;\n\tb=u74YJyanYG0kaNlMcrq9gE6iNR7s3irsuk07KDlHwODJeOWyhxXLdGL7Vsqk+V8Aq0\n\ti7NWdQrvsHv1U2RmGB8hoWDbbKdf3t1jHvUQhU8EDFf8U4BPXHEzknD44ApoVMKunxbb\n\tW3QcF0NXCeTL0/JRtad/LsFwcfiuK9UAiUDdJ6GV7BBQosjEOneaAjL/X0xizInBPrvS\n\tcwxauacP1DbwBZpArY1sdPJmZezIlFhB2n4iYWqwzt3DeR2MXAfjYriqjinNwVGR0Kl4\n\tFIi8hIEuqM6+5Ls9W/96w7tibBQ6evnc/o5p8ytWL5ajcCXKjbFpYk3XbO63y02uYZiW\n\tEvJA==","X-Gm-Message-State":"AOJu0YzpWIUHpZur1z5TYMA7TAvA6jsNtRB7l0lRy0GyFhhhPfgi+0RC\n\t0h+e7397jgklaS6k+BL7IdyIIQah+WXfguwXpWPzlavBS0VkTRY8pkN26379FviPBixPPY7XA3T\n\txi4csDAhRBa7wxGWu1tpLOBHsD0OfGPiX4KW5dlF3kzp0Vje5sG6Rm9/amwIRof/DX4O9NR0=","X-Received":["by 2002:ac2:485c:0:b0:513:c3:1d94 with SMTP id\n\t28-20020ac2485c000000b0051300c31d94mr6148359lfy.52.1710257508806; \n\tTue, 12 Mar 2024 08:31:48 -0700 (PDT)","by 2002:ac2:485c:0:b0:513:c3:1d94 with SMTP id\n\t28-20020ac2485c000000b0051300c31d94mr6148336lfy.52.1710257508328; \n\tTue, 12 Mar 2024 08:31:48 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHXgOWZt5LSSr9+gYhpGuW/v8qlMu9Rk+fSZpTPRo8QPlqmyerfjdZMgdlmNZ0rAKjdaxupxw==","From":"Milan Zamazal ","To":"Hans de Goede ","Subject":"Re: [PATCH v5 18/18] libcamera: Soft IPA: use CameraSensorHelper\n\tfor analogue gain","In-Reply-To":"<20240311141524.27192-19-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Mon, 11 Mar 2024 15:15:22 +0100\")","References":"<20240311141524.27192-1-hdegoede@redhat.com>\n\t<20240311141524.27192-19-hdegoede@redhat.com>","Date":"Tue, 12 Mar 2024 16:31:47 +0100","Message-ID":"<87zfv331r0.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":"Bryan O'Donoghue ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tPavel Machek ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]