[{"id":33189,"web_url":"https://patchwork.libcamera.org/comment/33189/","msgid":"<20250127012746.GL1133@pendragon.ideasonboard.com>","date":"2025-01-27T01:27:46","subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Mon, Jan 13, 2025 at 02:51:05PM +0100, Milan Zamazal wrote:\n> Applying color correction matrix (CCM) in software ISP is optional due\n> to performance reasons. CCM is applied if and only if `Ccm' algorithm\n> is present in the tuning file and it defines some CCM.\n> \n> Software ISP debayering is a performance critical piece of code and we\n> do not want to use dynamic conditionals there. Therefore we pass\n> information about CCM application to debayering configuration and let it\n> select the right versions of debayering functions using templates. This\n> is a similar trick as the previously used one for adding or not adding\n> an alpha channel to the output.\n> \n> Debayering gets this information but it ignores it in this patch.\n> Actual processing with CCM is added in the followup patch.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> include/libcamera/ipa/soft.mojom | 2 +-\n> src/ipa/simple/algorithms/ccm.cpp | 11 +++++--\n> src/ipa/simple/algorithms/ccm.h | 1 +\n> src/ipa/simple/soft_simple.cpp | 6 ++--\n> src/libcamera/software_isp/debayer.cpp | 3 +-\n> src/libcamera/software_isp/debayer.h | 3 +-\n> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++---------\n> src/libcamera/software_isp/debayer_cpu.h | 24 +++++++-------\n> src/libcamera/software_isp/software_isp.cpp | 5 +--\n> 9 files changed, 54 insertions(+), 36 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> index d52e6f1a..b5ed3905 100644\n> --- a/include/libcamera/ipa/soft.mojom\n> +++ b/include/libcamera/ipa/soft.mojom\n> @@ -21,7 +21,7 @@ interface IPASoftInterface {\n> \tstart() => (int32 ret);\n> \tstop();\n> \tconfigure(IPAConfigInfo configInfo)\n> -\t\t=> (int32 ret);\n> +\t\t=> (int32 ret, bool ccmEnabled);\n\nDo you foresee cases where the CCM algorithm could be enabled or\ndisabled depending on the camera configuration ? If not, I would return\nthis from the init() function instead of the configure() function.\n\n> \n> \t[async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);\n> \t[async] computeParams(uint32 frame);\n> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> index 3c7fca2d..96038966 100644\n> --- a/src/ipa/simple/algorithms/ccm.cpp\n> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> @@ -38,12 +38,17 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> \treturn 0;\n> }\n> \n> -void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> -\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> +int Ccm::configure(IPAContext &context,\n> +\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n\nI think you can skip this and handled the ccmEnabled variable purely in\nthe IPA module, but checking if the CCM algorithm has been instantiated.\nThis can be done by inspecting the algorithms list. Another option is to\nuse IPAACtiveState::ccm::enabled, by adding a default initializer to set\nit to false, and set it to true in Ccm::init().\n\n> {\n> \tcontext.activeState.ccm.enabled = ccmEnabled_;\n> +\treturn 0;\n> +}\n> \n> -\tif (!ccmEnabled_)\n> +void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> +{\n> +\tif (!context.activeState.ccm.enabled)\n> \t\treturn;\n> \n> \tunsigned int ct = context.activeState.awb.temperatureK;\n> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> index 23481a08..fe81ebd5 100644\n> --- a/src/ipa/simple/algorithms/ccm.h\n> +++ b/src/ipa/simple/algorithms/ccm.h\n> @@ -24,6 +24,7 @@ public:\n> \t~Ccm() = default;\n> \n> \tint init(IPAContext &context, const YamlObject &tuningData) override;\n> +\tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> \tvoid prepare(IPAContext &context,\n> \t\t const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index b26e4e15..9b32e02b 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -52,7 +52,7 @@ public:\n> \t\t const SharedFD &fdParams,\n> \t\t const ControlInfoMap &sensorInfoMap,\n> \t\t ControlInfoMap *ipaControls) override;\n> -\tint configure(const IPAConfigInfo &configInfo) override;\n> +\tint configure(const IPAConfigInfo &configInfo, bool *ccmEnabled) override;\n> \n> \tint start() override;\n> \tvoid stop() override;\n> @@ -182,7 +182,7 @@ int IPASoftSimple::init(const IPASettings &settings,\n> \treturn 0;\n> }\n> \n> -int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> +int IPASoftSimple::configure(const IPAConfigInfo &configInfo, bool *ccmEnabled)\n> {\n> \tsensorInfoMap_ = configInfo.sensorControls;\n> \n> @@ -248,6 +248,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> \t\t\treturn ret;\n> \t}\n> \n> +\t*ccmEnabled = context_.activeState.ccm.enabled;\n> +\n> \tLOG(IPASoft, Info)\n> \t\t<< \"Exposure \" << context_.configuration.agc.exposureMin << \"-\"\n> \t\t<< context_.configuration.agc.exposureMax\n> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n> index f0b83261..45fe6960 100644\n> --- a/src/libcamera/software_isp/debayer.cpp\n> +++ b/src/libcamera/software_isp/debayer.cpp\n> @@ -57,10 +57,11 @@ Debayer::~Debayer()\n> }\n> \n> /**\n> - * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs)\n> + * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs, bool ccmEnabled)\n> * \\brief Configure the debayer object according to the passed in parameters\n> * \\param[in] inputCfg The input configuration\n> * \\param[in] outputCfgs The output configurations\n> + * \\param[in] ccmEnabled Whether a color correction matrix is applied\n> *\n> * \\return 0 on success, a negative errno on failure\n> */\n> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h\n> index d7ca060d..ba033d44 100644\n> --- a/src/libcamera/software_isp/debayer.h\n> +++ b/src/libcamera/software_isp/debayer.h\n> @@ -33,7 +33,8 @@ public:\n> \tvirtual ~Debayer() = 0;\n> \n> \tvirtual int configure(const StreamConfiguration &inputCfg,\n> -\t\t\t const std::vector> &outputCfgs) = 0;\n> +\t\t\t const std::vector> &outputCfgs,\n> +\t\t\t bool ccmEnabled) = 0;\n> \n> \tvirtual std::vector formats(PixelFormat inputFormat) = 0;\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 01cfb36b..3c6597f9 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -114,7 +114,7 @@ DebayerCpu::~DebayerCpu() = default;\n> \t\t(prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div)), \\\n> \t\tcurr[x] / (div))\n> \n> -template\n> +template\n> void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tDECLARE_SRC_POINTERS(uint8_t)\n> @@ -125,7 +125,7 @@ void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tDECLARE_SRC_POINTERS(uint8_t)\n> @@ -136,7 +136,7 @@ void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tDECLARE_SRC_POINTERS(uint16_t)\n> @@ -148,7 +148,7 @@ void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tDECLARE_SRC_POINTERS(uint16_t)\n> @@ -160,7 +160,7 @@ void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tDECLARE_SRC_POINTERS(uint16_t)\n> @@ -172,7 +172,7 @@ void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tDECLARE_SRC_POINTERS(uint16_t)\n> @@ -184,7 +184,7 @@ void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tconst int widthInBytes = window_.width * 5 / 4;\n> @@ -210,7 +210,7 @@ void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tconst int widthInBytes = window_.width * 5 / 4;\n> @@ -231,7 +231,7 @@ void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tconst int widthInBytes = window_.width * 5 / 4;\n> @@ -252,7 +252,7 @@ void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n> \t}\n> }\n> \n> -template\n> +template\n> void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])\n> {\n> \tconst int widthInBytes = window_.width * 5 / 4;\n> @@ -368,10 +368,11 @@ int DebayerCpu::setupStandardBayerOrder(BayerFormat::Order order)\n> \treturn 0;\n> }\n> \n> -#define SET_DEBAYER_METHODS(method0, method1) \\\n> -\tdebayer0_ = addAlphaByte ? &DebayerCpu::method0 : &DebayerCpu::method0; \\\n> -\tdebayer1_ = addAlphaByte ? &DebayerCpu::method1 : &DebayerCpu::method1;\n> +#define SET_DEBAYER_METHODS(method0, method1) \\\n> +\tdebayer0_ = addAlphaByte ? &DebayerCpu::method0 : &DebayerCpu::method0; \\\n> +\tdebayer1_ = addAlphaByte ? &DebayerCpu::method1 : &DebayerCpu::method1;\n> \n> +template\n> int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat)\n\nThis function is not performance-critical, so you can pass ccmEnabled as\na function parameter. The SET_DEBAYER_METHODS() macro will need to\nbecome a bit more complex as a result, something along the lines of\n\n#define SET_DEBAYER_METHODS(method0, method1)\t\t\t\t\t\t\t\t\t\\\n\tdebayer0_ = addAlphaByte\t\t\t\t\t\t\t\t\t\t\\\n\t\t ? (ccmEnabled ? &DebayerCpu::method0 : &DebayerCpu::method0)\t\t\\\n\t\t : (ccmEnabled ? &DebayerCpu::method0 : &DebayerCpu::method0);\t\\\n\tdebayer1_ = addAlphaByte\t\t\t\t\t\t\t\t\t\t\\\n\t\t ? (ccmEnabled ? &DebayerCpu::method1 : &DebayerCpu::method1)\t\t\\\n\t\t : (ccmEnabled ? &DebayerCpu::method1 : &DebayerCpu::method1);\n\n> {\n> \tBayerFormat bayerFormat =\n> @@ -464,7 +465,8 @@ int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputF\n> }\n> \n> int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> -\t\t\t const std::vector> &outputCfgs)\n> +\t\t\t const std::vector> &outputCfgs,\n> +\t\t\t bool ccmEnabled)\n> {\n> \tif (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)\n> \t\treturn -EINVAL;\n> @@ -503,7 +505,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tif (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)\n> +\tint ret = ccmEnabled\n> +\t\t\t ? setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat)\n> +\t\t\t : setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat);\n> +\tif (ret != 0)\n> \t\treturn -EINVAL;\n> \n> \twindow_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 2c47e7c6..b2ec8f1b 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -31,7 +31,8 @@ public:\n> \t~DebayerCpu();\n> \n> \tint configure(const StreamConfiguration &inputCfg,\n> -\t\t const std::vector> &outputCfgs);\n> +\t\t const std::vector> &outputCfgs,\n> +\t\t bool ccmEnabled);\n> \tSize patternSize(PixelFormat inputFormat);\n> \tstd::vector formats(PixelFormat input);\n> \tstd::tuple\n> @@ -85,28 +86,28 @@ private:\n> \tusing debayerFn = void (DebayerCpu::*)(uint8_t *dst, const uint8_t *src[]);\n> \n> \t/* 8-bit raw bayer format */\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> \t/* unpacked 10-bit raw bayer format */\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> \t/* unpacked 12-bit raw bayer format */\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> \t/* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[]);\n> -\ttemplate\n> +\ttemplate\n> \tvoid debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> \n> \tstruct DebayerInputConfig {\n> @@ -125,6 +126,7 @@ private:\n> \tint getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);\n> \tint getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);\n> \tint setupStandardBayerOrder(BayerFormat::Order order);\n> +\ttemplate\n> \tint setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);\n> \tvoid setupInputMemcpy(const uint8_t *linePointers[]);\n> \tvoid shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index 2bea64d9..6a07de85 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -236,11 +236,12 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n> {\n> \tASSERT(ipa_ && debayer_);\n> \n> -\tint ret = ipa_->configure(configInfo);\n> +\tbool ccmEnabled;\n> +\tint ret = ipa_->configure(configInfo, &ccmEnabled);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\treturn debayer_->configure(inputCfg, outputCfgs);\n> +\treturn debayer_->configure(inputCfg, outputCfgs, ccmEnabled);\n> }\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 14B6EBEFBE\n\tfor ;\n\tMon, 27 Jan 2025 01:28:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 246426855D;\n\tMon, 27 Jan 2025 02:28:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4B8461875\n\tfor ;\n\tMon, 27 Jan 2025 02:27:57 +0100 (CET)","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 A2EAC352;\n\tMon, 27 Jan 2025 02:26:51 +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=\"mJRqC4R0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737941211;\n\tbh=yEIZ1U4OwGQAtYHCUVWvvmjMN+NFRpSIU/BRhJeBjFY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mJRqC4R0B+iSZjSvv1pVw6gRm1hemzfJ/GuaSTrsPlLl0wXQHQ496PNeAIJrpAQpW\n\t9tUf1TPCeSG0LV828qtFXCk9IqENTbHzSEUmwteraN9LvLNd/52aISUXEFltqQPL2I\n\tSVyciq472pN6ObMUvWMRyFHqxJlkniph2LZCgmg0=","Date":"Mon, 27 Jan 2025 03:27:46 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tRobert Mader ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","Message-ID":"<20250127012746.GL1133@pendragon.ideasonboard.com>","References":"<20250113135108.13924-1-mzamazal@redhat.com>\n\t<20250113135108.13924-9-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250113135108.13924-9-mzamazal@redhat.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":33219,"web_url":"https://patchwork.libcamera.org/comment/33219/","msgid":"<85ed0ndkz8.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-28T15:19:39","subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Mon, Jan 13, 2025 at 02:51:05PM +0100, Milan Zamazal wrote:\n>> Applying color correction matrix (CCM) in software ISP is optional due\n>> to performance reasons. CCM is applied if and only if `Ccm' algorithm\n>> is present in the tuning file and it defines some CCM.\n>> \n>> Software ISP debayering is a performance critical piece of code and we\n>> do not want to use dynamic conditionals there. Therefore we pass\n>> information about CCM application to debayering configuration and let it\n>> select the right versions of debayering functions using templates. This\n>> is a similar trick as the previously used one for adding or not adding\n>> an alpha channel to the output.\n>> \n>> Debayering gets this information but it ignores it in this patch.\n>> Actual processing with CCM is added in the followup patch.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> include/libcamera/ipa/soft.mojom | 2 +-\n>> src/ipa/simple/algorithms/ccm.cpp | 11 +++++--\n>> src/ipa/simple/algorithms/ccm.h | 1 +\n>> src/ipa/simple/soft_simple.cpp | 6 ++--\n>> src/libcamera/software_isp/debayer.cpp | 3 +-\n>> src/libcamera/software_isp/debayer.h | 3 +-\n>> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++---------\n>> src/libcamera/software_isp/debayer_cpu.h | 24 +++++++-------\n>> src/libcamera/software_isp/software_isp.cpp | 5 +--\n>> 9 files changed, 54 insertions(+), 36 deletions(-)\n>> \n>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n>> index d52e6f1a..b5ed3905 100644\n>> --- a/include/libcamera/ipa/soft.mojom\n>> +++ b/include/libcamera/ipa/soft.mojom\n>> @@ -21,7 +21,7 @@ interface IPASoftInterface {\n>> \tstart() => (int32 ret);\n>> \tstop();\n>> \tconfigure(IPAConfigInfo configInfo)\n>> -\t\t=> (int32 ret);\n>> +\t\t=> (int32 ret, bool ccmEnabled);\n>\n> Do you foresee cases where the CCM algorithm could be enabled or\n> disabled depending on the camera configuration ? If not, I would return\n> this from the init() function instead of the configure() function.\n\nOK, it's a bit more complicated to pass to the right place but possible.\nI'll do it in v5.\n\n>> \t[async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);\n>> \t[async] computeParams(uint32 frame);\n>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> index 3c7fca2d..96038966 100644\n>> --- a/src/ipa/simple/algorithms/ccm.cpp\n>> +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> @@ -38,12 +38,17 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n>> \treturn 0;\n>> }\n>> \n>> -void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>> -\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> +int Ccm::configure(IPAContext &context,\n>> +\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n>\n> I think you can skip this and handled the ccmEnabled variable purely in\n> the IPA module, but checking if the CCM algorithm has been instantiated.\n> This can be done by inspecting the algorithms list.\n\nI wouldn't like inspecting the algorithms list, I think it's cleaner to\nprovide the information from the algorithm itself. Moreover, other\nalgorithms need information about CCM. And finally, it cannot be\nhandled purely in the IPA module because it needs to be passed to\ndebayering (in software_isp.cpp).\n\n> Another option is to use IPAACtiveState::ccm::enabled, by adding a\n> default initializer to set it to false, and set it to true in\n> Ccm::init().\n\nIPAActiveState is reset in configure, there was a bug when it was\npreviously done in init. But it's possible to store the info elsewhere\nin the context.\n\n>> {\n>> \tcontext.activeState.ccm.enabled = ccmEnabled_;\n>> +\treturn 0;\n>> +}\n>> \n>> -\tif (!ccmEnabled_)\n>> +void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>> +\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> +{\n>> +\tif (!context.activeState.ccm.enabled)\n>> \t\treturn;\n>> \n>> \tunsigned int ct = context.activeState.awb.temperatureK;\n>> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n>> index 23481a08..fe81ebd5 100644\n>> --- a/src/ipa/simple/algorithms/ccm.h\n>> +++ b/src/ipa/simple/algorithms/ccm.h\n>> @@ -24,6 +24,7 @@ public:\n>> \t~Ccm() = default;\n>> \n>> \tint init(IPAContext &context, const YamlObject &tuningData) override;\n>> +\tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> \tvoid prepare(IPAContext &context,\n>> \t\t const uint32_t frame,\n>> \t\t IPAFrameContext &frameContext,\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index b26e4e15..9b32e02b 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -52,7 +52,7 @@ public:\n>> \t\t const SharedFD &fdParams,\n>> \t\t const ControlInfoMap &sensorInfoMap,\n>> \t\t ControlInfoMap *ipaControls) override;\n>> -\tint configure(const IPAConfigInfo &configInfo) override;\n>> +\tint configure(const IPAConfigInfo &configInfo, bool *ccmEnabled) override;\n>> \n>> \tint start() override;\n>> \tvoid stop() override;\n>> @@ -182,7 +182,7 @@ int IPASoftSimple::init(const IPASettings &settings,\n>> \treturn 0;\n>> }\n>> \n>> -int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>> +int IPASoftSimple::configure(const IPAConfigInfo &configInfo, bool *ccmEnabled)\n>> {\n>> \tsensorInfoMap_ = configInfo.sensorControls;\n>> \n>> @@ -248,6 +248,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>> \t\t\treturn ret;\n>> \t}\n>> \n>> +\t*ccmEnabled = context_.activeState.ccm.enabled;\n>> +\n>> \tLOG(IPASoft, Info)\n>> \t\t<< \"Exposure \" << context_.configuration.agc.exposureMin << \"-\"\n>> \t\t<< context_.configuration.agc.exposureMax\n>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n>> index f0b83261..45fe6960 100644\n>> --- a/src/libcamera/software_isp/debayer.cpp\n>> +++ b/src/libcamera/software_isp/debayer.cpp\n>> @@ -57,10 +57,11 @@ Debayer::~Debayer()\n>> }\n>> \n>> /**\n>> - * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs)\n>> + * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs, bool ccmEnabled)\n>> * \\brief Configure the debayer object according to the passed in parameters\n>> * \\param[in] inputCfg The input configuration\n>> * \\param[in] outputCfgs The output configurations\n>> + * \\param[in] ccmEnabled Whether a color correction matrix is applied\n>> *\n>> * \\return 0 on success, a negative errno on failure\n>> */\n>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h\n>> index d7ca060d..ba033d44 100644\n>> --- a/src/libcamera/software_isp/debayer.h\n>> +++ b/src/libcamera/software_isp/debayer.h\n>> @@ -33,7 +33,8 @@ public:\n>> \tvirtual ~Debayer() = 0;\n>> \n>> \tvirtual int configure(const StreamConfiguration &inputCfg,\n>> -\t\t\t const std::vector> &outputCfgs) = 0;\n>> +\t\t\t const std::vector> &outputCfgs,\n>> +\t\t\t bool ccmEnabled) = 0;\n>> \n>> \tvirtual std::vector formats(PixelFormat inputFormat) = 0;\n>> \n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 01cfb36b..3c6597f9 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -114,7 +114,7 @@ DebayerCpu::~DebayerCpu() = default;\n>> \t\t(prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div)), \\\n>> \t\tcurr[x] / (div))\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tDECLARE_SRC_POINTERS(uint8_t)\n>> @@ -125,7 +125,7 @@ void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tDECLARE_SRC_POINTERS(uint8_t)\n>> @@ -136,7 +136,7 @@ void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tDECLARE_SRC_POINTERS(uint16_t)\n>> @@ -148,7 +148,7 @@ void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tDECLARE_SRC_POINTERS(uint16_t)\n>> @@ -160,7 +160,7 @@ void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tDECLARE_SRC_POINTERS(uint16_t)\n>> @@ -172,7 +172,7 @@ void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tDECLARE_SRC_POINTERS(uint16_t)\n>> @@ -184,7 +184,7 @@ void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tconst int widthInBytes = window_.width * 5 / 4;\n>> @@ -210,7 +210,7 @@ void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tconst int widthInBytes = window_.width * 5 / 4;\n>> @@ -231,7 +231,7 @@ void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tconst int widthInBytes = window_.width * 5 / 4;\n>> @@ -252,7 +252,7 @@ void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n>> \t}\n>> }\n>> \n>> -template\n>> +template\n>> void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> {\n>> \tconst int widthInBytes = window_.width * 5 / 4;\n>> @@ -368,10 +368,11 @@ int DebayerCpu::setupStandardBayerOrder(BayerFormat::Order order)\n>> \treturn 0;\n>> }\n>> \n>> -#define SET_DEBAYER_METHODS(method0, method1) \\\n>> -\tdebayer0_ = addAlphaByte ? &DebayerCpu::method0 : &DebayerCpu::method0; \\\n>> -\tdebayer1_ = addAlphaByte ? &DebayerCpu::method1 : &DebayerCpu::method1;\n>> +#define SET_DEBAYER_METHODS(method0, method1) \\\n>> +\tdebayer0_ = addAlphaByte ? &DebayerCpu::method0 : &DebayerCpu::method0; \\\n>> +\tdebayer1_ = addAlphaByte ? &DebayerCpu::method1 : &DebayerCpu::method1;\n>> \n>> +template\n>> int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat)\n>\n> This function is not performance-critical, so you can pass ccmEnabled as\n> a function parameter. The SET_DEBAYER_METHODS() macro will need to\n> become a bit more complex as a result, something along the lines of\n>\n> #define SET_DEBAYER_METHODS(method0, method1)\t\t\t\t\t\t\t\t\t\\\n> \tdebayer0_ = addAlphaByte\t\t\t\t\t\t\t\t\t\t\\\n> \t\t ? (ccmEnabled ? &DebayerCpu::method0 : &DebayerCpu::method0)\t\t\\\n> \t\t : (ccmEnabled ? &DebayerCpu::method0 : &DebayerCpu::method0);\t\\\n> \tdebayer1_ = addAlphaByte\t\t\t\t\t\t\t\t\t\t\\\n> \t\t ? (ccmEnabled ? &DebayerCpu::method1 : &DebayerCpu::method1)\t\t\\\n> \t\t : (ccmEnabled ? &DebayerCpu::method1 : &DebayerCpu::method1);\n\nRight, will do in v5.\n\n>> {\n>> \tBayerFormat bayerFormat =\n>> @@ -464,7 +465,8 @@ int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputF\n>> }\n>> \n>> int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>> -\t\t\t const std::vector> &outputCfgs)\n>> +\t\t\t const std::vector> &outputCfgs,\n>> +\t\t\t bool ccmEnabled)\n>> {\n>> \tif (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)\n>> \t\treturn -EINVAL;\n>> @@ -503,7 +505,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> -\tif (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)\n>> +\tint ret = ccmEnabled\n>> +\t\t\t ? setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat)\n>> +\t\t\t : setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat);\n>> +\tif (ret != 0)\n>> \t\treturn -EINVAL;\n>> \n>> \twindow_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n>> index 2c47e7c6..b2ec8f1b 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> @@ -31,7 +31,8 @@ public:\n>> \t~DebayerCpu();\n>> \n>> \tint configure(const StreamConfiguration &inputCfg,\n>> -\t\t const std::vector> &outputCfgs);\n>> +\t\t const std::vector> &outputCfgs,\n>> +\t\t bool ccmEnabled);\n>> \tSize patternSize(PixelFormat inputFormat);\n>> \tstd::vector formats(PixelFormat input);\n>> \tstd::tuple\n>> @@ -85,28 +86,28 @@ private:\n>> \tusing debayerFn = void (DebayerCpu::*)(uint8_t *dst, const uint8_t *src[]);\n>> \n>> \t/* 8-bit raw bayer format */\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> \t/* unpacked 10-bit raw bayer format */\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> \t/* unpacked 12-bit raw bayer format */\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> \t/* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> -\ttemplate\n>> +\ttemplate\n>> \tvoid debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> \n>> \tstruct DebayerInputConfig {\n>> @@ -125,6 +126,7 @@ private:\n>> \tint getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);\n>> \tint getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);\n>> \tint setupStandardBayerOrder(BayerFormat::Order order);\n>> +\ttemplate\n>> \tint setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);\n>> \tvoid setupInputMemcpy(const uint8_t *linePointers[]);\n>> \tvoid shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> index 2bea64d9..6a07de85 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -236,11 +236,12 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n>> {\n>> \tASSERT(ipa_ && debayer_);\n>> \n>> -\tint ret = ipa_->configure(configInfo);\n>> +\tbool ccmEnabled;\n>> +\tint ret = ipa_->configure(configInfo, &ccmEnabled);\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>> \n>> -\treturn debayer_->configure(inputCfg, outputCfgs);\n>> +\treturn debayer_->configure(inputCfg, outputCfgs, ccmEnabled);\n>> }\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 3EBAEBD87C\n\tfor ;\n\tTue, 28 Jan 2025 15:19:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 860D66855D;\n\tTue, 28 Jan 2025 16:19:49 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DD0368546\n\tfor ;\n\tTue, 28 Jan 2025 16:19:47 +0100 (CET)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-33-fhxTq4O8ORurfHBvQ4My0Q-1; Tue, 28 Jan 2025 10:19:42 -0500","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-3862b364578so3222177f8f.1\n\tfor ;\n\tTue, 28 Jan 2025 07:19:42 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb (nat-pool-brq-t.redhat.com.\n\t[213.175.37.10]) by smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-38c2a17d74esm14744409f8f.37.2025.01.28.07.19.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 28 Jan 2025 07:19:40 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"SzrKsp7a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738077586;\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=jWbXy6M9ls9BpRBcepI4YEef/BqpJEjsujO3Dybc47U=;\n\tb=SzrKsp7agTbyKitqBL8Kd7iCGVPVh5NxBZRZIbMPJZ3PMOGPpYQRDZ0Xoo8BsbZ/VfLwvQ\n\thpedE7+gJwdf/lho0A0NnE3zKDSBOYsRyCgSnRkE7Rxm85aE276F+9plXW3eZvYz0wAw/4\n\t+PEIGrZmQuuDloT212r/IdMJG7yjPGQ=","X-MC-Unique":"fhxTq4O8ORurfHBvQ4My0Q-1","X-Mimecast-MFC-AGG-ID":"fhxTq4O8ORurfHBvQ4My0Q","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738077581; x=1738682381;\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=jWbXy6M9ls9BpRBcepI4YEef/BqpJEjsujO3Dybc47U=;\n\tb=ITivZkW1BlyoqC9T6BCASGO3PDKM7HvbjBV4kiQB553OzlvuidPxP0eufREr/ainXn\n\tbq9jpI7yP21zHYPkEgW2/AjxS0JwpTfGzG/aVNPFUczxuAY0hl7PCMRc9kKP90WOx+Zh\n\t67v7S/tjdYBXFhR7t6GNUlqTK+wiXRyEljoPvTkXiKW0bskxknSMSiOboKtn3tWNYPIm\n\tvNhJwKR2sFS3mXrmw84cs5zQJIlOlqpdEcQqaRbrqpywihiOzLpa4S1mnrCM2uyJA9Eu\n\tDKdpumMvhN52y6kswnV14II/t47Mp8vBhyUQg5CrZzpq+J19emeVcYVN+Nz/FfMJbWFy\n\tWU7A==","X-Gm-Message-State":"AOJu0Yxcgt9tdMaPqgevttCUHviZJ+o/fdPVLP0LbLqswUGrKBnYcc2S\n\tdbFtv1LnUd3ly/6TlvmqAmBVDnLYyAilBD9jh1A+HlLd+wsibeRc1iAscnXV7t5vuwSKjGEq6Ln\n\teRMYD0NH5hj1ofo3ljG7NYdQ0zIGj8oePd8CeyLYLSwmeL03t38QUG4T9TSIJ+wRHWpSNahk=","X-Gm-Gg":"ASbGncsONXa5MKHUF7N/xGVXp9dqAlT1wBr02w+BpKHeKblu3HSWMr3IcfhaSMwhrNo\n\tOin4S6042ctIl1hvAELv9wOJUIJTYWOZCHhjdq5b++tIvdpXVUb9eokyHcs86HYptONucDT6FVY\n\trVbZwcAQ8AceOOz5plFz4jVMuf/N/YrSCaZUSBiuSM1RUYTC8fO334+JzspQ+tC/LAWsFr6rFAH\n\txa0/Rj+THHWBF+h3CPCPV7cwz/RPi63JriPZ9bsfCADd3l1YmJRzses5JWSmcEeHJOEKE7JtK/Z\n\tlKVBj2ScrcBQbDmUge5pO7Hmm4fTVHSzKHn1hs3t1pV24Ln+L5PpdRbk","X-Received":["by 2002:a5d:598e:0:b0:38c:24c6:5bdc with SMTP id\n\tffacd0b85a97d-38c49a1cf43mr3374848f8f.4.1738077581324; \n\tTue, 28 Jan 2025 07:19:41 -0800 (PST)","by 2002:a5d:598e:0:b0:38c:24c6:5bdc with SMTP id\n\tffacd0b85a97d-38c49a1cf43mr3374820f8f.4.1738077580859; \n\tTue, 28 Jan 2025 07:19:40 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFHi9MzD4XONz5SEd+V3dMyjYDcM3suJx1vH7UEIgiXPBPlXy0TEZV59C3smhI7UIrcI4i9xg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Robert Mader\n\t, Hans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","In-Reply-To":"<20250127012746.GL1133@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 27 Jan 2025 03:27:46 +0200\")","References":"<20250113135108.13924-1-mzamazal@redhat.com>\n\t<20250113135108.13924-9-mzamazal@redhat.com>\n\t<20250127012746.GL1133@pendragon.ideasonboard.com>","Date":"Tue, 28 Jan 2025 16:19:39 +0100","Message-ID":"<85ed0ndkz8.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"H6Mz-oI7gGlKVLY-AwwEKHzEgqRpgPsbgZ-midVHBOI_1738077581","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33220,"web_url":"https://patchwork.libcamera.org/comment/33220/","msgid":"<20250128152955.GB12673@pendragon.ideasonboard.com>","date":"2025-01-28T15:29:55","subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Tue, Jan 28, 2025 at 04:19:39PM +0100, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Mon, Jan 13, 2025 at 02:51:05PM +0100, Milan Zamazal wrote:\n> >> Applying color correction matrix (CCM) in software ISP is optional due\n> >> to performance reasons. CCM is applied if and only if `Ccm' algorithm\n> >> is present in the tuning file and it defines some CCM.\n> >> \n> >> Software ISP debayering is a performance critical piece of code and we\n> >> do not want to use dynamic conditionals there. Therefore we pass\n> >> information about CCM application to debayering configuration and let it\n> >> select the right versions of debayering functions using templates. This\n> >> is a similar trick as the previously used one for adding or not adding\n> >> an alpha channel to the output.\n> >> \n> >> Debayering gets this information but it ignores it in this patch.\n> >> Actual processing with CCM is added in the followup patch.\n> >> \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> include/libcamera/ipa/soft.mojom | 2 +-\n> >> src/ipa/simple/algorithms/ccm.cpp | 11 +++++--\n> >> src/ipa/simple/algorithms/ccm.h | 1 +\n> >> src/ipa/simple/soft_simple.cpp | 6 ++--\n> >> src/libcamera/software_isp/debayer.cpp | 3 +-\n> >> src/libcamera/software_isp/debayer.h | 3 +-\n> >> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++---------\n> >> src/libcamera/software_isp/debayer_cpu.h | 24 +++++++-------\n> >> src/libcamera/software_isp/software_isp.cpp | 5 +--\n> >> 9 files changed, 54 insertions(+), 36 deletions(-)\n> >> \n> >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> >> index d52e6f1a..b5ed3905 100644\n> >> --- a/include/libcamera/ipa/soft.mojom\n> >> +++ b/include/libcamera/ipa/soft.mojom\n> >> @@ -21,7 +21,7 @@ interface IPASoftInterface {\n> >> \tstart() => (int32 ret);\n> >> \tstop();\n> >> \tconfigure(IPAConfigInfo configInfo)\n> >> -\t\t=> (int32 ret);\n> >> +\t\t=> (int32 ret, bool ccmEnabled);\n> >\n> > Do you foresee cases where the CCM algorithm could be enabled or\n> > disabled depending on the camera configuration ? If not, I would return\n> > this from the init() function instead of the configure() function.\n> \n> OK, it's a bit more complicated to pass to the right place but possible.\n> I'll do it in v5.\n> \n> >> \t[async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);\n> >> \t[async] computeParams(uint32 frame);\n> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> >> index 3c7fca2d..96038966 100644\n> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> >> @@ -38,12 +38,17 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> >> \treturn 0;\n> >> }\n> >> \n> >> -void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> >> -\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> >> +int Ccm::configure(IPAContext &context,\n> >> +\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >\n> > I think you can skip this and handled the ccmEnabled variable purely in\n> > the IPA module, but checking if the CCM algorithm has been instantiated.\n> > This can be done by inspecting the algorithms list.\n> \n> I wouldn't like inspecting the algorithms list, I think it's cleaner to\n> provide the information from the algorithm itself. Moreover, other\n> algorithms need information about CCM.\n\nYou need to support the case where the CCM algorithm is not listed in\nthe tuning file, so the information has to be handled in the IPA module\nitself. If the need is to check if a CCM algorithm has been\ninstantiated, then looking at the algorithms list seems the best fit.\n\n> And finally, it cannot be\n> handled purely in the IPA module because it needs to be passed to\n> debayering (in software_isp.cpp).\n\nThat's not what I meant, I was talking about how to check if CCM needs\nto be applied. The information of course has to be passed to the\npipeline handler.\n\nAnd now that I wrote this, I realize it would be good to also consider\nhow this will be hanlded when sharing \"ISP\" configuration buffers\nbetween the IPA module and the pipeline handler. The CCM configuration\ncould then possibly vary per-frame.\n\n> > Another option is to use IPAACtiveState::ccm::enabled, by adding a\n> > default initializer to set it to false, and set it to true in\n> > Ccm::init().\n> \n> IPAActiveState is reset in configure, there was a bug when it was\n> previously done in init. But it's possible to store the info elsewhere\n> in the context.\n> \n> >> {\n> >> \tcontext.activeState.ccm.enabled = ccmEnabled_;\n> >> +\treturn 0;\n> >> +}\n> >> \n> >> -\tif (!ccmEnabled_)\n> >> +void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> >> +\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> >> +{\n> >> +\tif (!context.activeState.ccm.enabled)\n> >> \t\treturn;\n> >> \n> >> \tunsigned int ct = context.activeState.awb.temperatureK;\n> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> >> index 23481a08..fe81ebd5 100644\n> >> --- a/src/ipa/simple/algorithms/ccm.h\n> >> +++ b/src/ipa/simple/algorithms/ccm.h\n> >> @@ -24,6 +24,7 @@ public:\n> >> \t~Ccm() = default;\n> >> \n> >> \tint init(IPAContext &context, const YamlObject &tuningData) override;\n> >> +\tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> >> \tvoid prepare(IPAContext &context,\n> >> \t\t const uint32_t frame,\n> >> \t\t IPAFrameContext &frameContext,\n> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> >> index b26e4e15..9b32e02b 100644\n> >> --- a/src/ipa/simple/soft_simple.cpp\n> >> +++ b/src/ipa/simple/soft_simple.cpp\n> >> @@ -52,7 +52,7 @@ public:\n> >> \t\t const SharedFD &fdParams,\n> >> \t\t const ControlInfoMap &sensorInfoMap,\n> >> \t\t ControlInfoMap *ipaControls) override;\n> >> -\tint configure(const IPAConfigInfo &configInfo) override;\n> >> +\tint configure(const IPAConfigInfo &configInfo, bool *ccmEnabled) override;\n> >> \n> >> \tint start() override;\n> >> \tvoid stop() override;\n> >> @@ -182,7 +182,7 @@ int IPASoftSimple::init(const IPASettings &settings,\n> >> \treturn 0;\n> >> }\n> >> \n> >> -int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> >> +int IPASoftSimple::configure(const IPAConfigInfo &configInfo, bool *ccmEnabled)\n> >> {\n> >> \tsensorInfoMap_ = configInfo.sensorControls;\n> >> \n> >> @@ -248,6 +248,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> >> \t\t\treturn ret;\n> >> \t}\n> >> \n> >> +\t*ccmEnabled = context_.activeState.ccm.enabled;\n> >> +\n> >> \tLOG(IPASoft, Info)\n> >> \t\t<< \"Exposure \" << context_.configuration.agc.exposureMin << \"-\"\n> >> \t\t<< context_.configuration.agc.exposureMax\n> >> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n> >> index f0b83261..45fe6960 100644\n> >> --- a/src/libcamera/software_isp/debayer.cpp\n> >> +++ b/src/libcamera/software_isp/debayer.cpp\n> >> @@ -57,10 +57,11 @@ Debayer::~Debayer()\n> >> }\n> >> \n> >> /**\n> >> - * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs)\n> >> + * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs, bool ccmEnabled)\n> >> * \\brief Configure the debayer object according to the passed in parameters\n> >> * \\param[in] inputCfg The input configuration\n> >> * \\param[in] outputCfgs The output configurations\n> >> + * \\param[in] ccmEnabled Whether a color correction matrix is applied\n> >> *\n> >> * \\return 0 on success, a negative errno on failure\n> >> */\n> >> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h\n> >> index d7ca060d..ba033d44 100644\n> >> --- a/src/libcamera/software_isp/debayer.h\n> >> +++ b/src/libcamera/software_isp/debayer.h\n> >> @@ -33,7 +33,8 @@ public:\n> >> \tvirtual ~Debayer() = 0;\n> >> \n> >> \tvirtual int configure(const StreamConfiguration &inputCfg,\n> >> -\t\t\t const std::vector> &outputCfgs) = 0;\n> >> +\t\t\t const std::vector> &outputCfgs,\n> >> +\t\t\t bool ccmEnabled) = 0;\n> >> \n> >> \tvirtual std::vector formats(PixelFormat inputFormat) = 0;\n> >> \n> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> >> index 01cfb36b..3c6597f9 100644\n> >> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> >> @@ -114,7 +114,7 @@ DebayerCpu::~DebayerCpu() = default;\n> >> \t\t(prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div)), \\\n> >> \t\tcurr[x] / (div))\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tDECLARE_SRC_POINTERS(uint8_t)\n> >> @@ -125,7 +125,7 @@ void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tDECLARE_SRC_POINTERS(uint8_t)\n> >> @@ -136,7 +136,7 @@ void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tDECLARE_SRC_POINTERS(uint16_t)\n> >> @@ -148,7 +148,7 @@ void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tDECLARE_SRC_POINTERS(uint16_t)\n> >> @@ -160,7 +160,7 @@ void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tDECLARE_SRC_POINTERS(uint16_t)\n> >> @@ -172,7 +172,7 @@ void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tDECLARE_SRC_POINTERS(uint16_t)\n> >> @@ -184,7 +184,7 @@ void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tconst int widthInBytes = window_.width * 5 / 4;\n> >> @@ -210,7 +210,7 @@ void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tconst int widthInBytes = window_.width * 5 / 4;\n> >> @@ -231,7 +231,7 @@ void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tconst int widthInBytes = window_.width * 5 / 4;\n> >> @@ -252,7 +252,7 @@ void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> \t}\n> >> }\n> >> \n> >> -template\n> >> +template\n> >> void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> {\n> >> \tconst int widthInBytes = window_.width * 5 / 4;\n> >> @@ -368,10 +368,11 @@ int DebayerCpu::setupStandardBayerOrder(BayerFormat::Order order)\n> >> \treturn 0;\n> >> }\n> >> \n> >> -#define SET_DEBAYER_METHODS(method0, method1) \\\n> >> -\tdebayer0_ = addAlphaByte ? &DebayerCpu::method0 : &DebayerCpu::method0; \\\n> >> -\tdebayer1_ = addAlphaByte ? &DebayerCpu::method1 : &DebayerCpu::method1;\n> >> +#define SET_DEBAYER_METHODS(method0, method1) \\\n> >> +\tdebayer0_ = addAlphaByte ? &DebayerCpu::method0 : &DebayerCpu::method0; \\\n> >> +\tdebayer1_ = addAlphaByte ? &DebayerCpu::method1 : &DebayerCpu::method1;\n> >> \n> >> +template\n> >> int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat)\n> >\n> > This function is not performance-critical, so you can pass ccmEnabled as\n> > a function parameter. The SET_DEBAYER_METHODS() macro will need to\n> > become a bit more complex as a result, something along the lines of\n> >\n> > #define SET_DEBAYER_METHODS(method0, method1)\t\t\t\t\t\t\t\t\t\\\n> > \tdebayer0_ = addAlphaByte\t\t\t\t\t\t\t\t\t\t\\\n> > \t\t ? (ccmEnabled ? &DebayerCpu::method0 : &DebayerCpu::method0)\t\t\\\n> > \t\t : (ccmEnabled ? &DebayerCpu::method0 : &DebayerCpu::method0);\t\\\n> > \tdebayer1_ = addAlphaByte\t\t\t\t\t\t\t\t\t\t\\\n> > \t\t ? (ccmEnabled ? &DebayerCpu::method1 : &DebayerCpu::method1)\t\t\\\n> > \t\t : (ccmEnabled ? &DebayerCpu::method1 : &DebayerCpu::method1);\n> \n> Right, will do in v5.\n> \n> >> {\n> >> \tBayerFormat bayerFormat =\n> >> @@ -464,7 +465,8 @@ int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputF\n> >> }\n> >> \n> >> int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> >> -\t\t\t const std::vector> &outputCfgs)\n> >> +\t\t\t const std::vector> &outputCfgs,\n> >> +\t\t\t bool ccmEnabled)\n> >> {\n> >> \tif (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)\n> >> \t\treturn -EINVAL;\n> >> @@ -503,7 +505,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> -\tif (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)\n> >> +\tint ret = ccmEnabled\n> >> +\t\t\t ? setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat)\n> >> +\t\t\t : setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat);\n> >> +\tif (ret != 0)\n> >> \t\treturn -EINVAL;\n> >> \n> >> \twindow_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &\n> >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> >> index 2c47e7c6..b2ec8f1b 100644\n> >> --- a/src/libcamera/software_isp/debayer_cpu.h\n> >> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> >> @@ -31,7 +31,8 @@ public:\n> >> \t~DebayerCpu();\n> >> \n> >> \tint configure(const StreamConfiguration &inputCfg,\n> >> -\t\t const std::vector> &outputCfgs);\n> >> +\t\t const std::vector> &outputCfgs,\n> >> +\t\t bool ccmEnabled);\n> >> \tSize patternSize(PixelFormat inputFormat);\n> >> \tstd::vector formats(PixelFormat input);\n> >> \tstd::tuple\n> >> @@ -85,28 +86,28 @@ private:\n> >> \tusing debayerFn = void (DebayerCpu::*)(uint8_t *dst, const uint8_t *src[]);\n> >> \n> >> \t/* 8-bit raw bayer format */\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> \t/* unpacked 10-bit raw bayer format */\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> \t/* unpacked 12-bit raw bayer format */\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> \t/* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> -\ttemplate\n> >> +\ttemplate\n> >> \tvoid debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> >> \n> >> \tstruct DebayerInputConfig {\n> >> @@ -125,6 +126,7 @@ private:\n> >> \tint getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);\n> >> \tint getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);\n> >> \tint setupStandardBayerOrder(BayerFormat::Order order);\n> >> +\ttemplate\n> >> \tint setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);\n> >> \tvoid setupInputMemcpy(const uint8_t *linePointers[]);\n> >> \tvoid shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> >> index 2bea64d9..6a07de85 100644\n> >> --- a/src/libcamera/software_isp/software_isp.cpp\n> >> +++ b/src/libcamera/software_isp/software_isp.cpp\n> >> @@ -236,11 +236,12 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n> >> {\n> >> \tASSERT(ipa_ && debayer_);\n> >> \n> >> -\tint ret = ipa_->configure(configInfo);\n> >> +\tbool ccmEnabled;\n> >> +\tint ret = ipa_->configure(configInfo, &ccmEnabled);\n> >> \tif (ret < 0)\n> >> \t\treturn ret;\n> >> \n> >> -\treturn debayer_->configure(inputCfg, outputCfgs);\n> >> +\treturn debayer_->configure(inputCfg, outputCfgs, ccmEnabled);\n> >> }\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 74370BDB13\n\tfor ;\n\tTue, 28 Jan 2025 15:30:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B029B6855D;\n\tTue, 28 Jan 2025 16:30:08 +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 754FA68546\n\tfor ;\n\tTue, 28 Jan 2025 16:30:06 +0100 (CET)","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 118343A4;\n\tTue, 28 Jan 2025 16:28:59 +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=\"WQWUYMfe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738078139;\n\tbh=UbAaao/T8pHFxaqppL8FjmwjLDV/ULlMA9dC1Db7H0I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WQWUYMfeZcAGvWpEO/IkKNdgQi5oEeEsNk3nqz9wddI5FdpSmxX4WkQC8sm2hysMl\n\t6Y3IRbwa61K65lZcqcmB0So8GIp/GXcX48n3/XviEwU1C/RCWMWM6UGtNq7IIDA6GI\n\tAXMUztnz3gLvsPW+gXuJi2JkqHGS1pq5sUcsXJAw=","Date":"Tue, 28 Jan 2025 17:29:55 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tRobert Mader ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","Message-ID":"<20250128152955.GB12673@pendragon.ideasonboard.com>","References":"<20250113135108.13924-1-mzamazal@redhat.com>\n\t<20250113135108.13924-9-mzamazal@redhat.com>\n\t<20250127012746.GL1133@pendragon.ideasonboard.com>\n\t<85ed0ndkz8.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<85ed0ndkz8.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","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":33224,"web_url":"https://patchwork.libcamera.org/comment/33224/","msgid":"<85o6zpnbja.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-29T10:45:45","subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> On Tue, Jan 28, 2025 at 04:19:39PM +0100, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>> > On Mon, Jan 13, 2025 at 02:51:05PM +0100, Milan Zamazal wrote:\n>> >> Applying color correction matrix (CCM) in software ISP is optional due\n>> >> to performance reasons. CCM is applied if and only if `Ccm' algorithm\n>> >> is present in the tuning file and it defines some CCM.\n>> >> \n>> >> Software ISP debayering is a performance critical piece of code and we\n>> >> do not want to use dynamic conditionals there. Therefore we pass\n>> >> information about CCM application to debayering configuration and let it\n>> >> select the right versions of debayering functions using templates. This\n>> >> is a similar trick as the previously used one for adding or not adding\n>> >> an alpha channel to the output.\n>> >> \n>> >> Debayering gets this information but it ignores it in this patch.\n>> >> Actual processing with CCM is added in the followup patch.\n>> >> \n>> >> Signed-off-by: Milan Zamazal \n>> >> ---\n>> >> include/libcamera/ipa/soft.mojom | 2 +-\n>> >> src/ipa/simple/algorithms/ccm.cpp | 11 +++++--\n>> >> src/ipa/simple/algorithms/ccm.h | 1 +\n>> >> src/ipa/simple/soft_simple.cpp | 6 ++--\n>> >> src/libcamera/software_isp/debayer.cpp | 3 +-\n>> >> src/libcamera/software_isp/debayer.h | 3 +-\n>> >> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++---------\n>> >> src/libcamera/software_isp/debayer_cpu.h | 24 +++++++-------\n>> >> src/libcamera/software_isp/software_isp.cpp | 5 +--\n>> >> 9 files changed, 54 insertions(+), 36 deletions(-)\n>> >> \n>> >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n>> >> index d52e6f1a..b5ed3905 100644\n>> >> --- a/include/libcamera/ipa/soft.mojom\n>> >> +++ b/include/libcamera/ipa/soft.mojom\n>> >> @@ -21,7 +21,7 @@ interface IPASoftInterface {\n>> >> \tstart() => (int32 ret);\n>> >> \tstop();\n>> >> \tconfigure(IPAConfigInfo configInfo)\n>> >> -\t\t=> (int32 ret);\n>> >> +\t\t=> (int32 ret, bool ccmEnabled);\n>> >\n>> > Do you foresee cases where the CCM algorithm could be enabled or\n>> > disabled depending on the camera configuration ? If not, I would return\n>> > this from the init() function instead of the configure() function.\n>> \n>> OK, it's a bit more complicated to pass to the right place but possible.\n>> I'll do it in v5.\n>> \n>> >> \t[async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);\n>> >> \t[async] computeParams(uint32 frame);\n>> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> >> index 3c7fca2d..96038966 100644\n>> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n>> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> >> @@ -38,12 +38,17 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n>> >> \treturn 0;\n>> >> }\n>> >> \n>> >> -void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>> >> -\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> >> +int Ccm::configure(IPAContext &context,\n>> >> +\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> >\n>> > I think you can skip this and handled the ccmEnabled variable purely in\n>> > the IPA module, but checking if the CCM algorithm has been instantiated.\n>> > This can be done by inspecting the algorithms list.\n>> \n>> I wouldn't like inspecting the algorithms list, I think it's cleaner to\n>> provide the information from the algorithm itself. Moreover, other\n>> algorithms need information about CCM.\n>\n> You need to support the case where the CCM algorithm is not listed in\n> the tuning file, so the information has to be handled in the IPA module\n> itself. If the need is to check if a CCM algorithm has been\n> instantiated, then looking at the algorithms list seems the best fit.\n\nWhy is it better to check the list of the algorithms rather than the\nalgorithm self-reporting itself by setting a flag in the context, which\nmust be done anyway so that the other algorithms know (and the IPA\nmodule just passes this info to software ISP)?\n\n>> And finally, it cannot be\n>> handled purely in the IPA module because it needs to be passed to\n>> debayering (in software_isp.cpp).\n>\n> That's not what I meant, I was talking about how to check if CCM needs\n> to be applied. The information of course has to be passed to the\n> pipeline handler.\n>\n> And now that I wrote this, I realize it would be good to also consider\n> how this will be hanlded when sharing \"ISP\" configuration buffers\n> between the IPA module and the pipeline handler. The CCM configuration\n> could then possibly vary per-frame.\n\nI'm not sure I get the point. The overall CCM configuration (whether\nenabled and the matrices in the tuning file) cannot change. The\nparticular matrices to be applied for each frame are represented by the\nlookup tables created in Lut algorithm (based on inputs from Ccm\nalgorithm) and passed in debayering parameters the same way as before\nwith non-CCM lookup tables only, nothing changes in that really. What's\nthe problem then?\n\n>> > Another option is to use IPAACtiveState::ccm::enabled, by adding a\n>> > default initializer to set it to false, and set it to true in\n>> > Ccm::init().\n>> \n>> IPAActiveState is reset in configure, there was a bug when it was\n>> previously done in init. But it's possible to store the info elsewhere\n>> in the context.\n>> \n>> >> {\n>> >> \tcontext.activeState.ccm.enabled = ccmEnabled_;\n>> >> +\treturn 0;\n>> >> +}\n>> >> \n>> >> -\tif (!ccmEnabled_)\n>> >> +void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>> >> +\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> >> +{\n>> >> +\tif (!context.activeState.ccm.enabled)\n>> >> \t\treturn;\n>> >> \n>> >> \tunsigned int ct = context.activeState.awb.temperatureK;\n>> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n>> >> index 23481a08..fe81ebd5 100644\n>> >> --- a/src/ipa/simple/algorithms/ccm.h\n>> >> +++ b/src/ipa/simple/algorithms/ccm.h\n>> >> @@ -24,6 +24,7 @@ public:\n>> >> \t~Ccm() = default;\n>> >> \n>> >> \tint init(IPAContext &context, const YamlObject &tuningData) override;\n>> >> +\tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> >> \tvoid prepare(IPAContext &context,\n>> >> \t\t const uint32_t frame,\n>> >> \t\t IPAFrameContext &frameContext,\n>> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> >> index b26e4e15..9b32e02b 100644\n>> >> --- a/src/ipa/simple/soft_simple.cpp\n>> >> +++ b/src/ipa/simple/soft_simple.cpp\n>> >> @@ -52,7 +52,7 @@ public:\n>> >> \t\t const SharedFD &fdParams,\n>> >> \t\t const ControlInfoMap &sensorInfoMap,\n>> >> \t\t ControlInfoMap *ipaControls) override;\n>> >> -\tint configure(const IPAConfigInfo &configInfo) override;\n>> >> +\tint configure(const IPAConfigInfo &configInfo, bool *ccmEnabled) override;\n>> >> \n>> >> \tint start() override;\n>> >> \tvoid stop() override;\n>> >> @@ -182,7 +182,7 @@ int IPASoftSimple::init(const IPASettings &settings,\n>> >> \treturn 0;\n>> >> }\n>> >> \n>> >> -int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>> >> +int IPASoftSimple::configure(const IPAConfigInfo &configInfo, bool *ccmEnabled)\n>> >> {\n>> >> \tsensorInfoMap_ = configInfo.sensorControls;\n>> >> \n>> >> @@ -248,6 +248,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>> >> \t\t\treturn ret;\n>> >> \t}\n>> >> \n>> >> +\t*ccmEnabled = context_.activeState.ccm.enabled;\n>> >> +\n>> >> \tLOG(IPASoft, Info)\n>> >> \t\t<< \"Exposure \" << context_.configuration.agc.exposureMin << \"-\"\n>> >> \t\t<< context_.configuration.agc.exposureMax\n>> >> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n>> >> index f0b83261..45fe6960 100644\n>> >> --- a/src/libcamera/software_isp/debayer.cpp\n>> >> +++ b/src/libcamera/software_isp/debayer.cpp\n>> >> @@ -57,10 +57,11 @@ Debayer::~Debayer()\n>> >> }\n>> >> \n>> >> /**\n>> >> - * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs)\n>> >> + * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs, bool ccmEnabled)\n>> >> * \\brief Configure the debayer object according to the passed in parameters\n>> >> * \\param[in] inputCfg The input configuration\n>> >> * \\param[in] outputCfgs The output configurations\n>> >> + * \\param[in] ccmEnabled Whether a color correction matrix is applied\n>> >> *\n>> >> * \\return 0 on success, a negative errno on failure\n>> >> */\n>> >> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h\n>> >> index d7ca060d..ba033d44 100644\n>> >> --- a/src/libcamera/software_isp/debayer.h\n>> >> +++ b/src/libcamera/software_isp/debayer.h\n>> >> @@ -33,7 +33,8 @@ public:\n>> >> \tvirtual ~Debayer() = 0;\n>> >> \n>> >> \tvirtual int configure(const StreamConfiguration &inputCfg,\n>> >> -\t\t\t const std::vector> &outputCfgs) = 0;\n>> >> +\t\t\t const std::vector> &outputCfgs,\n>> >> +\t\t\t bool ccmEnabled) = 0;\n>> >> \n>> >> \tvirtual std::vector formats(PixelFormat inputFormat) = 0;\n>> >> \n>> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> >> index 01cfb36b..3c6597f9 100644\n>> >> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> >> @@ -114,7 +114,7 @@ DebayerCpu::~DebayerCpu() = default;\n>> >> \t\t(prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div)), \\\n>> >> \t\tcurr[x] / (div))\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tDECLARE_SRC_POINTERS(uint8_t)\n>> >> @@ -125,7 +125,7 @@ void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tDECLARE_SRC_POINTERS(uint8_t)\n>> >> @@ -136,7 +136,7 @@ void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tDECLARE_SRC_POINTERS(uint16_t)\n>> >> @@ -148,7 +148,7 @@ void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tDECLARE_SRC_POINTERS(uint16_t)\n>> >> @@ -160,7 +160,7 @@ void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tDECLARE_SRC_POINTERS(uint16_t)\n>> >> @@ -172,7 +172,7 @@ void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tDECLARE_SRC_POINTERS(uint16_t)\n>> >> @@ -184,7 +184,7 @@ void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tconst int widthInBytes = window_.width * 5 / 4;\n>> >> @@ -210,7 +210,7 @@ void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tconst int widthInBytes = window_.width * 5 / 4;\n>> >> @@ -231,7 +231,7 @@ void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tconst int widthInBytes = window_.width * 5 / 4;\n>> >> @@ -252,7 +252,7 @@ void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> \t}\n>> >> }\n>> >> \n>> >> -template\n>> >> +template\n>> >> void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> >> {\n>> >> \tconst int widthInBytes = window_.width * 5 / 4;\n>> >> @@ -368,10 +368,11 @@ int DebayerCpu::setupStandardBayerOrder(BayerFormat::Order order)\n>> >> \treturn 0;\n>> >> }\n>> >> \n>> >> -#define SET_DEBAYER_METHODS(method0, method1) \\\n>> >> -\tdebayer0_ = addAlphaByte ? &DebayerCpu::method0 : &DebayerCpu::method0; \\\n>> >> -\tdebayer1_ = addAlphaByte ? &DebayerCpu::method1 : &DebayerCpu::method1;\n>> >> +#define SET_DEBAYER_METHODS(method0, method1) \\\n>> >> +\tdebayer0_ = addAlphaByte ? &DebayerCpu::method0 : &DebayerCpu::method0; \\\n>> >> +\tdebayer1_ = addAlphaByte ? &DebayerCpu::method1 : &DebayerCpu::method1;\n>> >> \n>> >> +template\n>> >> int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat)\n>> >\n>> > This function is not performance-critical, so you can pass ccmEnabled as\n>> > a function parameter. The SET_DEBAYER_METHODS() macro will need to\n>> > become a bit more complex as a result, something along the lines of\n>> >\n>> > #define SET_DEBAYER_METHODS(method0, method1)\t\t\t\t\t\t\t\t\t\\\n>> > \tdebayer0_ = addAlphaByte\t\t\t\t\t\t\t\t\t\t\\\n>> > \t\t ? (ccmEnabled ? &DebayerCpu::method0 : &DebayerCpu::method0)\t\t\\\n>> > \t\t : (ccmEnabled ? &DebayerCpu::method0 : &DebayerCpu::method0);\t\\\n>> > \tdebayer1_ = addAlphaByte\t\t\t\t\t\t\t\t\t\t\\\n>> > \t\t ? (ccmEnabled ? &DebayerCpu::method1 : &DebayerCpu::method1)\t\t\\\n>> > \t\t : (ccmEnabled ? &DebayerCpu::method1 : &DebayerCpu::method1);\n>> \n>> Right, will do in v5.\n>> \n>> >> {\n>> >> \tBayerFormat bayerFormat =\n>> >> @@ -464,7 +465,8 @@ int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputF\n>> >> }\n>> >> \n>> >> int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>> >> -\t\t\t const std::vector> &outputCfgs)\n>> >> +\t\t\t const std::vector> &outputCfgs,\n>> >> +\t\t\t bool ccmEnabled)\n>> >> {\n>> >> \tif (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)\n>> >> \t\treturn -EINVAL;\n>> >> @@ -503,7 +505,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>> >> \t\treturn -EINVAL;\n>> >> \t}\n>> >> \n>> >> -\tif (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)\n>> >> +\tint ret = ccmEnabled\n>> >> +\t\t\t ? setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat)\n>> >> +\t\t\t : setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat);\n>> >> +\tif (ret != 0)\n>> >> \t\treturn -EINVAL;\n>> >> \n>> >> \twindow_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &\n>> >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n>> >> index 2c47e7c6..b2ec8f1b 100644\n>> >> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> >> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> >> @@ -31,7 +31,8 @@ public:\n>> >> \t~DebayerCpu();\n>> >> \n>> >> \tint configure(const StreamConfiguration &inputCfg,\n>> >> -\t\t const std::vector> &outputCfgs);\n>> >> +\t\t const std::vector> &outputCfgs,\n>> >> +\t\t bool ccmEnabled);\n>> >> \tSize patternSize(PixelFormat inputFormat);\n>> >> \tstd::vector formats(PixelFormat input);\n>> >> \tstd::tuple\n>> >> @@ -85,28 +86,28 @@ private:\n>> >> \tusing debayerFn = void (DebayerCpu::*)(uint8_t *dst, const uint8_t *src[]);\n>> >> \n>> >> \t/* 8-bit raw bayer format */\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> \t/* unpacked 10-bit raw bayer format */\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> \t/* unpacked 12-bit raw bayer format */\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> \t/* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> -\ttemplate\n>> >> +\ttemplate\n>> >> \tvoid debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);\n>> >> \n>> >> \tstruct DebayerInputConfig {\n>> >> @@ -125,6 +126,7 @@ private:\n>> >> \tint getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);\n>> >> \tint getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);\n>> >> \tint setupStandardBayerOrder(BayerFormat::Order order);\n>> >> +\ttemplate\n>> >> \tint setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);\n>> >> \tvoid setupInputMemcpy(const uint8_t *linePointers[]);\n>> >> \tvoid shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n>> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> >> index 2bea64d9..6a07de85 100644\n>> >> --- a/src/libcamera/software_isp/software_isp.cpp\n>> >> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> >> @@ -236,11 +236,12 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n>> >> {\n>> >> \tASSERT(ipa_ && debayer_);\n>> >> \n>> >> -\tint ret = ipa_->configure(configInfo);\n>> >> +\tbool ccmEnabled;\n>> >> +\tint ret = ipa_->configure(configInfo, &ccmEnabled);\n>> >> \tif (ret < 0)\n>> >> \t\treturn ret;\n>> >> \n>> >> -\treturn debayer_->configure(inputCfg, outputCfgs);\n>> >> +\treturn debayer_->configure(inputCfg, outputCfgs, ccmEnabled);\n>> >> }\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 1BC18C324E\n\tfor ;\n\tWed, 29 Jan 2025 10:45:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B10686855D;\n\tWed, 29 Jan 2025 11:45:54 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33EE561877\n\tfor ;\n\tWed, 29 Jan 2025 11:45:52 +0100 (CET)","from mail-wr1-f72.google.com (mail-wr1-f72.google.com\n\t[209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-689--Ps9da_4MUaLP8JGDteRlg-1; Wed, 29 Jan 2025 05:45:49 -0500","by mail-wr1-f72.google.com with SMTP id\n\tffacd0b85a97d-385e3cbf308so2461609f8f.2\n\tfor ;\n\tWed, 29 Jan 2025 02:45:49 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-38c2a17d6b2sm17079747f8f.34.2025.01.29.02.45.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 29 Jan 2025 02:45:46 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Y8wngcvh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738147551;\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=+k4VtIDIeeQxpKK3+EgAo45JJfyqOHyuK+K8eYUKk1g=;\n\tb=Y8wngcvhUu4emOklQGZ3vvL3bgKCZSxuZ/M0vCj2BkR994TKmnVcXDaBcN1KXqN7Go5n66\n\tC49yQkSrXypjqEPtcUPkiVQapbLRMI5SkOHRrDo13TBtlxyKcp2IJeMyzs4/FhJpzPzPHE\n\tv9dt/uSJ8VBuym7OU4osC/5P4mWBpt8=","X-MC-Unique":"-Ps9da_4MUaLP8JGDteRlg-1","X-Mimecast-MFC-AGG-ID":"-Ps9da_4MUaLP8JGDteRlg","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738147548; x=1738752348;\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=+k4VtIDIeeQxpKK3+EgAo45JJfyqOHyuK+K8eYUKk1g=;\n\tb=Vdt4cFytBVT82tAGUysPjMxnjSVlfV69KRqBQVLHjxxro1xx9qI4MnE7t7LxtNs/LW\n\trkPgu8OgC4KA8iUuDOxDlNEMunSsintLP/WAMUgafdeRhFJLZa3ozWU39tJSoTASCO3B\n\taNGasQd+0YNqHnxS6qRXKko1bq33Y8mqskUNqCFJ+/HDyAy0nvE8sbhcGXiJu5J56V28\n\tNQoMAL+Bqcncz4PbPupMlQUKZAOViIu9ziRFyGEvozkGJk6meLmkaLzpS+kvKKi5lw1E\n\tdfEwfeHiQW8rtHoGusFDkasDBc7q7MpffPYOqpaiHVk0buZyVA3XHSoBvqnjF8ReO6fV\n\tC1Yg==","X-Gm-Message-State":"AOJu0YzpeGjnZwKPxUfPMFHThOna8TfS5Ur2aJ4APtQwxjaLQWsMI3RV\n\tt3Sq5TvUJAAcJt5jQ7kew0CUqhxPVVK2wk+xTFowt4aGFSh3AFmrybrdCYH0nBLOTB4NzecB+O3\n\tvXqV4b11U+O6ULPMwIsL/sxf2wTx/s+PC9L6yMY1wc1GxvxFN/HhmNgeI0OqqgNc0uqNI96w=","X-Gm-Gg":"ASbGnct1WQTsWkykyIOJsAK9hHjVvZjX1on7A5Ke4bw6HMO4kt7+MWhLPmfj+1qUVan\n\ty9+9wgitKBeeLcZoAYGMnCPbdEYu9HjeSepIU4l1AYBfgqPD7VxleEynZNwzznnjybCsECOXAOS\n\tu5JFPb91h1nAnQYt0Zi3bAb+EbpJzp3Q2a3FIJbXV500pKqa0ihG3tnwL/fYpvkO2vo7dTdjZ9B\n\tHrWsVAtcG+VXf/uRNi7yMudalMpKj0BqoDrTPQM7eEIjRhWzYIh0EOGrcwbu7E91dgUshoAwL1Z\n\tJAXhaKIamx6X3qBMl6xXm7pAMoFLvGXi8BTefzO9+yKsuSvzzndg0ja/0Q==","X-Received":["by 2002:a5d:6d8f:0:b0:385:ef2f:92ad with SMTP id\n\tffacd0b85a97d-38c51931727mr2080032f8f.10.1738147547869; \n\tWed, 29 Jan 2025 02:45:47 -0800 (PST)","by 2002:a5d:6d8f:0:b0:385:ef2f:92ad with SMTP id\n\tffacd0b85a97d-38c51931727mr2080010f8f.10.1738147547417; \n\tWed, 29 Jan 2025 02:45:47 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFfhl7yDl5ik4vAv54oHOJvdYsGHi4RtV0H/BqeNvKwTP9yl79PWF2+BboH2ztDiUKdCQR6yQ==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Robert Mader\n\t, Hans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","In-Reply-To":"<20250128152955.GB12673@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Tue, 28 Jan 2025 17:29:55 +0200\")","References":"<20250113135108.13924-1-mzamazal@redhat.com>\n\t<20250113135108.13924-9-mzamazal@redhat.com>\n\t<20250127012746.GL1133@pendragon.ideasonboard.com>\n\t<85ed0ndkz8.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t<20250128152955.GB12673@pendragon.ideasonboard.com>","Date":"Wed, 29 Jan 2025 11:45:45 +0100","Message-ID":"<85o6zpnbja.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"uQaGlyohl-DWxy32d6sLWBTzYrgUuQlfymufMGqSceQ_1738147548","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33225,"web_url":"https://patchwork.libcamera.org/comment/33225/","msgid":"<173814822593.1594000.139662176791275026@ping.linuxembedded.co.uk>","date":"2025-01-29T10:57:05","subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-01-29 10:45:45)\n> Hi Laurent,\n> \n> Laurent Pinchart writes:\n> \n> > Hi Milan,\n> >\n> > On Tue, Jan 28, 2025 at 04:19:39PM +0100, Milan Zamazal wrote:\n> >> Laurent Pinchart writes:\n> >> > On Mon, Jan 13, 2025 at 02:51:05PM +0100, Milan Zamazal wrote:\n> >> >> Applying color correction matrix (CCM) in software ISP is optional due\n> >> >> to performance reasons. CCM is applied if and only if `Ccm' algorithm\n> >> >> is present in the tuning file and it defines some CCM.\n> >> >> \n> >> >> Software ISP debayering is a performance critical piece of code and we\n> >> >> do not want to use dynamic conditionals there. Therefore we pass\n> >> >> information about CCM application to debayering configuration and let it\n> >> >> select the right versions of debayering functions using templates. This\n> >> >> is a similar trick as the previously used one for adding or not adding\n> >> >> an alpha channel to the output.\n> >> >> \n> >> >> Debayering gets this information but it ignores it in this patch.\n> >> >> Actual processing with CCM is added in the followup patch.\n> >> >> \n> >> >> Signed-off-by: Milan Zamazal \n> >> >> ---\n> >> >> include/libcamera/ipa/soft.mojom | 2 +-\n> >> >> src/ipa/simple/algorithms/ccm.cpp | 11 +++++--\n> >> >> src/ipa/simple/algorithms/ccm.h | 1 +\n> >> >> src/ipa/simple/soft_simple.cpp | 6 ++--\n> >> >> src/libcamera/software_isp/debayer.cpp | 3 +-\n> >> >> src/libcamera/software_isp/debayer.h | 3 +-\n> >> >> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++---------\n> >> >> src/libcamera/software_isp/debayer_cpu.h | 24 +++++++-------\n> >> >> src/libcamera/software_isp/software_isp.cpp | 5 +--\n> >> >> 9 files changed, 54 insertions(+), 36 deletions(-)\n> >> >> \n> >> >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> >> >> index d52e6f1a..b5ed3905 100644\n> >> >> --- a/include/libcamera/ipa/soft.mojom\n> >> >> +++ b/include/libcamera/ipa/soft.mojom\n> >> >> @@ -21,7 +21,7 @@ interface IPASoftInterface {\n> >> >> start() => (int32 ret);\n> >> >> stop();\n> >> >> configure(IPAConfigInfo configInfo)\n> >> >> - => (int32 ret);\n> >> >> + => (int32 ret, bool ccmEnabled);\n> >> >\n> >> > Do you foresee cases where the CCM algorithm could be enabled or\n> >> > disabled depending on the camera configuration ? If not, I would return\n> >> > this from the init() function instead of the configure() function.\n> >> \n> >> OK, it's a bit more complicated to pass to the right place but possible.\n> >> I'll do it in v5.\n> >> \n> >> >> [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);\n> >> >> [async] computeParams(uint32 frame);\n> >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> >> >> index 3c7fca2d..96038966 100644\n> >> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n> >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> >> >> @@ -38,12 +38,17 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> >> >> return 0;\n> >> >> }\n> >> >> \n> >> >> -void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> >> >> - IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> >> >> +int Ccm::configure(IPAContext &context,\n> >> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >> >\n> >> > I think you can skip this and handled the ccmEnabled variable purely in\n> >> > the IPA module, but checking if the CCM algorithm has been instantiated.\n> >> > This can be done by inspecting the algorithms list.\n> >> \n> >> I wouldn't like inspecting the algorithms list, I think it's cleaner to\n> >> provide the information from the algorithm itself. Moreover, other\n> >> algorithms need information about CCM.\n> >\n> > You need to support the case where the CCM algorithm is not listed in\n> > the tuning file, so the information has to be handled in the IPA module\n> > itself. If the need is to check if a CCM algorithm has been\n> > instantiated, then looking at the algorithms list seems the best fit.\n> \n> Why is it better to check the list of the algorithms rather than the\n> algorithm self-reporting itself by setting a flag in the context, which\n> must be done anyway so that the other algorithms know (and the IPA\n> module just passes this info to software ISP)?\n\nCan you check what happens when there is no ccm: section in the tuning\nfile ?\n\nI expect no ccm: in tuning file will mean no CCM algorithm gets\nloaded/executed at all in the IPA. That's how the other libipa modules\nwork.\n\n--\nKieran","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 6285BBD808\n\tfor ;\n\tWed, 29 Jan 2025 10:57:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E6D26855D;\n\tWed, 29 Jan 2025 11:57:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1FB961877\n\tfor ;\n\tWed, 29 Jan 2025 11:57:08 +0100 (CET)","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 DC272D21;\n\tWed, 29 Jan 2025 11:56:00 +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=\"TwD0solx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738148161;\n\tbh=XDxfhQyfTC+/6H3v/K1/NOU2qFqtVX3fggMfblUYsdM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=TwD0solxKyIdm8FVi684c6MC/N2I2jvp/b4YhoBmFj8mNOj5wOgpsyL5vj9Bvunib\n\tPLMauPcfNsCi/K9HNsg7mXMkrYaDwKdJhovrolN3ZsPWSqx7Ed0ECghaxGFQEqu6Vu\n\tobZad5FlzwHHxuBTOq/rrl/JltsLB9Oi8zUBUrsg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<85o6zpnbja.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","References":"<20250113135108.13924-1-mzamazal@redhat.com>\n\t<20250113135108.13924-9-mzamazal@redhat.com>\n\t<20250127012746.GL1133@pendragon.ideasonboard.com>\n\t<85ed0ndkz8.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t<20250128152955.GB12673@pendragon.ideasonboard.com>\n\t<85o6zpnbja.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","Subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tRobert Mader ,\n\tHans de Goede ","To":"Laurent Pinchart ,\n\tMilan Zamazal ","Date":"Wed, 29 Jan 2025 10:57:05 +0000","Message-ID":"<173814822593.1594000.139662176791275026@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":33228,"web_url":"https://patchwork.libcamera.org/comment/33228/","msgid":"<85jzadn4c1.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-29T13:21:18","subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Milan Zamazal (2025-01-29 10:45:45)\n>> Hi Laurent,\n>> \n>\n>> Laurent Pinchart writes:\n>> \n>> > Hi Milan,\n>> >\n>> > On Tue, Jan 28, 2025 at 04:19:39PM +0100, Milan Zamazal wrote:\n>> >> Laurent Pinchart writes:\n>> >> > On Mon, Jan 13, 2025 at 02:51:05PM +0100, Milan Zamazal wrote:\n>> >> >> Applying color correction matrix (CCM) in software ISP is optional due\n>> >> >> to performance reasons. CCM is applied if and only if `Ccm' algorithm\n>> >> >> is present in the tuning file and it defines some CCM.\n>> >> >> \n>> >> >> Software ISP debayering is a performance critical piece of code and we\n>> >> >> do not want to use dynamic conditionals there. Therefore we pass\n>> >> >> information about CCM application to debayering configuration and let it\n>> >> >> select the right versions of debayering functions using templates. This\n>> >> >> is a similar trick as the previously used one for adding or not adding\n>> >> >> an alpha channel to the output.\n>> >> >> \n>> >> >> Debayering gets this information but it ignores it in this patch.\n>> >> >> Actual processing with CCM is added in the followup patch.\n>> >> >> \n>> >> >> Signed-off-by: Milan Zamazal \n>> >> >> ---\n>> >> >> include/libcamera/ipa/soft.mojom | 2 +-\n>> >> >> src/ipa/simple/algorithms/ccm.cpp | 11 +++++--\n>> >> >> src/ipa/simple/algorithms/ccm.h | 1 +\n>> >> >> src/ipa/simple/soft_simple.cpp | 6 ++--\n>> >> >> src/libcamera/software_isp/debayer.cpp | 3 +-\n>> >> >> src/libcamera/software_isp/debayer.h | 3 +-\n>> >> >> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++---------\n>> >> >> src/libcamera/software_isp/debayer_cpu.h | 24 +++++++-------\n>> >> >> src/libcamera/software_isp/software_isp.cpp | 5 +--\n>> >> >> 9 files changed, 54 insertions(+), 36 deletions(-)\n>> >> >> \n>> >> >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n>> >> >> index d52e6f1a..b5ed3905 100644\n>> >> >> --- a/include/libcamera/ipa/soft.mojom\n>> >> >> +++ b/include/libcamera/ipa/soft.mojom\n>> >> >> @@ -21,7 +21,7 @@ interface IPASoftInterface {\n>> >> >> start() => (int32 ret);\n>> >> >> stop();\n>> >> >> configure(IPAConfigInfo configInfo)\n>> >> >> - => (int32 ret);\n>> >> >> + => (int32 ret, bool ccmEnabled);\n>> >> >\n>> >> > Do you foresee cases where the CCM algorithm could be enabled or\n>> >> > disabled depending on the camera configuration ? If not, I would return\n>> >> > this from the init() function instead of the configure() function.\n>> >> \n>> >> OK, it's a bit more complicated to pass to the right place but possible.\n>> >> I'll do it in v5.\n>> >> \n>> >> >> [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);\n>> >> >> [async] computeParams(uint32 frame);\n>> >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> >> >> index 3c7fca2d..96038966 100644\n>> >> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n>> >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> >> >> @@ -38,12 +38,17 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n>> >> >> return 0;\n>> >> >> }\n>> >> >> \n>> >> >> -void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>> >> >> - IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> >> >> +int Ccm::configure(IPAContext &context,\n>> >> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> >> >\n>> >> > I think you can skip this and handled the ccmEnabled variable purely in\n>> >> > the IPA module, but checking if the CCM algorithm has been instantiated.\n>> >> > This can be done by inspecting the algorithms list.\n>> >> \n>> >> I wouldn't like inspecting the algorithms list, I think it's cleaner to\n>> >> provide the information from the algorithm itself. Moreover, other\n>> >> algorithms need information about CCM.\n>> >\n>> > You need to support the case where the CCM algorithm is not listed in\n>> > the tuning file, so the information has to be handled in the IPA module\n>> > itself. If the need is to check if a CCM algorithm has been\n>> > instantiated, then looking at the algorithms list seems the best fit.\n>> \n>> Why is it better to check the list of the algorithms rather than the\n>> algorithm self-reporting itself by setting a flag in the context, which\n>> must be done anyway so that the other algorithms know (and the IPA\n>> module just passes this info to software ISP)?\n>\n> Can you check what happens when there is no ccm: section in the tuning\n> file ?\n>\n> I expect no ccm: in tuning file will mean no CCM algorithm gets\n> loaded/executed at all in the IPA. That's how the other libipa modules\n> work.\n\nYes, that's the expected behavior. Then ccmEnabled flag remains false\n(the default value) and things work accordingly, mostly the same as\nbefore this patch set.","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 98588C32C2\n\tfor ;\n\tWed, 29 Jan 2025 13:21:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8110B6855C;\n\tWed, 29 Jan 2025 14:21:26 +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 D21836851B\n\tfor ;\n\tWed, 29 Jan 2025 14:21:24 +0100 (CET)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-322-uvmCT29QMzmJ6BWJ6Gq9Gw-1; Wed, 29 Jan 2025 08:21:22 -0500","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-aa67855b3deso596557066b.1\n\tfor ;\n\tWed, 29 Jan 2025 05:21:22 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-ab675e64e9bsm966172166b.46.2025.01.29.05.21.19\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 29 Jan 2025 05:21:19 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"V+MPbfHb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738156883;\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=5UfqAzItxhxtIzxg0O9Dlqw5BBLNE6WeITVt5fO8eZ4=;\n\tb=V+MPbfHbdEGIiSldByXgybKaQinUT/3Ioc6vONa4f9Mn+69dnSYO/3rLnhhrrMzNOnfAtn\n\tIP9EMNuvzzlK/yRe1nx3mOGqWf6EvE+rHPtXDKIcsd+X3Rj05VmAm6eCMO3Y2JeCQ31w57\n\t8fyOaLSrgtBLy8PrNcCqyj7S1FSO5Ek=","X-MC-Unique":"uvmCT29QMzmJ6BWJ6Gq9Gw-1","X-Mimecast-MFC-AGG-ID":"uvmCT29QMzmJ6BWJ6Gq9Gw","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738156881; x=1738761681;\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=5UfqAzItxhxtIzxg0O9Dlqw5BBLNE6WeITVt5fO8eZ4=;\n\tb=DPCXWQc54HHswuPTadMOD2/QmQou4AoOdwJMmrdIt+ARmMatAFWP3vbqgihf1iR70T\n\tt0VixCEyIdZvP9/KVay1/VWLyjH2fDDqiYOPcWx9sZ9j3dO9kdcWVrdvbAPyGlEs4SZQ\n\tzISTOSMbGXRk6zXMj3P9EgMTF5ZiB6AcQ9AUDPp1I1qR1bs9+4a0hFx9aJhkY278o0mx\n\tgxn68ABuKs9V2rDGnx5FxRsGFcsc2tcx/ZO3kYRz7rUzZ5wl+r3SwoUL2Npw4610ibWC\n\tvkaCd/MikwNV1stCx3tIqsRGrekK69WP+puigClc6g6TUANrk1pQb8HBt6IeqJlmrk6w\n\tBVyw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVyAgeoQKaoTIJ9FpuLJLD+k4gqQUQSZ7/wA2cYYunefkxiFVOIQotbcoV81geEqktZclLC4j8i2JZT+wISTek=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzDoceGJdOGdvGBqYEktAiOuQsl/Yy3j+k06MzETCcq4qJJ25Ha\n\t0brRQiNlBwQ0+nZhCmD++b7CCwn+RruhUadnk0+Ctjmlh8sH/qvmSduFP0bZNVHGYVFIDKjnunl\n\tWkTBQhGJsWSJKRtaT4DwO4FD2hBwoyXwlDd3Q8TbJ9LdHOPpTqb+5t4TmwiJ5ws6QhCQhxOhEOj\n\tAzNe0=","X-Gm-Gg":"ASbGnct0mTUmJUd9/cK9U3rwjW8mxrNxiMdMjkBZB+QvlDIXyZzZoBp/Ev+RaBa29Cw\n\tqD/8IoXzsttybh7eaiXvnMPbEiQMYekvsNJh7AAcmuR9SJZ3mpETnyMyFXx4oVgzUNodyRYkX4r\n\toDvUYvwijgiQIEH9TtGwHWmVod9KkvgBRiszkrcS2txRtF/W+Bo3QPDBnBKDMs9dFkCaaDHbkUs\n\tSO1bwN6Cl/l1H9dfHhoeLzHT+Lh1Ur/j5V9/wXTZqDO8CtVkqINkVDguPhglZ1XIv6jqXyRVTkO\n\tjkvQXYpC2VmNi9n35/yRo37o4ivt557PjxI7h/8I03U/d4bcgfXB/S3pmg==","X-Received":["by 2002:a17:907:6eaa:b0:aaf:afb3:ad63 with SMTP id\n\ta640c23a62f3a-ab6cfdbe573mr260142866b.43.1738156880674; \n\tWed, 29 Jan 2025 05:21:20 -0800 (PST)","by 2002:a17:907:6eaa:b0:aaf:afb3:ad63 with SMTP id\n\ta640c23a62f3a-ab6cfdbe573mr260139866b.43.1738156880190; \n\tWed, 29 Jan 2025 05:21:20 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFoCO2Feyz7yzY+H0ifO9nZ35x3X84IqYeqCBU4caFM4bQilcNAD6s3Ckx8wLRSz+PQ1TcuTw==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, Robert Mader\n\t, Hans de Goede ","Subject":"Re: [PATCH v4 8/9] libcamera: software_isp: Track whether CCM is\n\tenabled","In-Reply-To":"<173814822593.1594000.139662176791275026@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Wed, 29 Jan 2025 10:57:05 +0000\")","References":"<20250113135108.13924-1-mzamazal@redhat.com>\n\t<20250113135108.13924-9-mzamazal@redhat.com>\n\t<20250127012746.GL1133@pendragon.ideasonboard.com>\n\t<85ed0ndkz8.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t<20250128152955.GB12673@pendragon.ideasonboard.com>\n\t<85o6zpnbja.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t<173814822593.1594000.139662176791275026@ping.linuxembedded.co.uk>","Date":"Wed, 29 Jan 2025 14:21:18 +0100","Message-ID":"<85jzadn4c1.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"4HhBMLkWda0MiDZ-3AFwxTk5GHKHTR3sVWHw7cJjk5k_1738156881","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]