[{"id":32829,"web_url":"https://patchwork.libcamera.org/comment/32829/","msgid":"<173445297887.32744.14806654289831116215@ping.linuxembedded.co.uk>","date":"2024-12-17T16:29:38","subject":"Re: [RFC PATCH] ipa: rkisp1: Initialize lsc tables on first frame","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-12-16 15:33:02)\n> When sending the parameters for lens shading correction to the ISP on\n> frame x (in the test mostly frame 4) stalls of the ISP were observed.\n> From userland the symptom is that no buffers get returned anymore.\n> Kernel wise the ISP constantly produces interrupts with\n> RKISP1_CIF_ISP_V_START being set but no interrupts with\n> RKISP1_CIF_ISP_FRAME. It turned out that this behavior wasn't observed\n> when the lens shading correction parameters were written before\n> receiving the first frame. Ensure that behavior in the lsc algorithm by\n> unconditionally filling the params buffer on frame 0.\n> \n> Unfortunately there were also cases were no stalls were observed even\n> though the lsc tables were not written on frame 0. So it is not clear\n> if this fix fixes the actual root cause.\n> \n> Another reason for that change is that it is sensible to write the lsc\n> data on frame 0, to minimize the flicker in the image when the first\n> statistics come in (where they would have been written otherwise).\n\nI think it's reasonable to write an LSC calibration table from the\nstart. It might be difficult to ascertain 'which one' if we have an\noption to interpolate this based on color temperature, but I would\nsuspect any of the pre-calibrated tables with the wrong temperature are\nbetter than an identity mapping for the starting configuration.\n\n> Signed-off-by: Stefan Klug \n> ---\n> \n> src/ipa/rkisp1/algorithms/lsc.cpp | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> index e47aa2f0727e..15a43799ae27 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> @@ -410,12 +410,20 @@ void LensShadingCorrection::prepare(IPAContext &context,\n> RkISP1Params *params)\n> {\n> uint32_t ct = context.activeState.awb.temperatureK;\n\nOn frame zero - what is this initialised to ? Is it something sane? Is\nit '0' ? And mostly is it something that won't badly affect the code below?\n\nIf the LSC interpolations get clamped to sane ranges, then I would be\nhappy with this - but I can't tell from the context of this patch alone,\nhowever I do suspect that's the case so:\n\n\nReviewed-by: Kieran Bingham \n\n> - if (std::abs(static_cast(ct) - static_cast(lastAppliedCt_)) <\n> + /*\n> + * The ISP in the imx8mp using linux 6.12 has a high probability for a\n> + * stall when the lsc module isn't configured in the first frame.\n> + * Therefore ensure that a lsc table is written unconditionally at the\n> + * first frame.\n> + */\n> + if (frame > 0 &&\n> + std::abs(static_cast(ct) - static_cast(lastAppliedCt_)) <\n> kColourTemperatureChangeThreshhold)\n> return;\n> +\n> unsigned int quantizedCt;\n> const Components &set = sets_.getInterpolated(ct, &quantizedCt);\n> - if (lastAppliedQuantizedCt_ == quantizedCt)\n> + if (frame > 0 && lastAppliedQuantizedCt_ == quantizedCt)\n> return;\n> \n> auto config = params->block();\n> -- \n> 2.43.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7563EBD1F1\n\tfor ;\n\tTue, 17 Dec 2024 16:29:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7580F67FE5;\n\tTue, 17 Dec 2024 17:29:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ABCF267FDF\n\tfor ;\n\tTue, 17 Dec 2024 17:29:41 +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 480C34C7;\n\tTue, 17 Dec 2024 17:29:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YQ0cBaig\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734452944;\n\tbh=KwukWle5Kk4pZWqFPYTrEbT/4D/omd9vk4Dpc/g75pI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=YQ0cBaigajbFcmVPcVhPvMOBXy2UIPo8Y+iMJAx3CXGNq0j6U+pbbvXn0Qss/lAtZ\n\tkWIZZu3SJE2kW9Lsb95A5WRr6acKFrTXn4WNFPnOw72lRbDQsahAo3kvTzawUZBvRX\n\tgmmDfZnYB/L7VzPv9yf802lpsUCI1MzejFc12kwg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241216153344.201302-1-stefan.klug@ideasonboard.com>","References":"<20241216153344.201302-1-stefan.klug@ideasonboard.com>","Subject":"Re: [RFC PATCH] ipa: rkisp1: Initialize lsc tables on first frame","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 17 Dec 2024 16:29:38 +0000","Message-ID":"<173445297887.32744.14806654289831116215@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":32842,"web_url":"https://patchwork.libcamera.org/comment/32842/","msgid":"<20241217170451.GL20432@pendragon.ideasonboard.com>","date":"2024-12-17T17:04:51","subject":"Re: [RFC PATCH] ipa: rkisp1: Initialize lsc tables on first frame","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Dec 17, 2024 at 04:29:38PM +0000, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-12-16 15:33:02)\n> > When sending the parameters for lens shading correction to the ISP on\n> > frame x (in the test mostly frame 4) stalls of the ISP were observed.\n> > From userland the symptom is that no buffers get returned anymore.\n> > Kernel wise the ISP constantly produces interrupts with\n> > RKISP1_CIF_ISP_V_START being set but no interrupts with\n> > RKISP1_CIF_ISP_FRAME. It turned out that this behavior wasn't observed\n> > when the lens shading correction parameters were written before\n> > receiving the first frame. Ensure that behavior in the lsc algorithm by\n> > unconditionally filling the params buffer on frame 0.\n> > \n> > Unfortunately there were also cases were no stalls were observed even\n> > though the lsc tables were not written on frame 0. So it is not clear\n> > if this fix fixes the actual root cause.\n> > \n> > Another reason for that change is that it is sensible to write the lsc\n> > data on frame 0, to minimize the flicker in the image when the first\n> > statistics come in (where they would have been written otherwise).\n> \n> I think it's reasonable to write an LSC calibration table from the\n> start. It might be difficult to ascertain 'which one' if we have an\n> option to interpolate this based on color temperature, but I would\n> suspect any of the pre-calibrated tables with the wrong temperature are\n> better than an identity mapping for the starting configuration.\n\nI agree. However, I'm concerned that the kernel makes it possible to\nhand the ISP so easily.\n\n> > Signed-off-by: Stefan Klug \n> > ---\n> > \n> > src/ipa/rkisp1/algorithms/lsc.cpp | 12 ++++++++++--\n> > 1 file changed, 10 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > index e47aa2f0727e..15a43799ae27 100644\n> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > @@ -410,12 +410,20 @@ void LensShadingCorrection::prepare(IPAContext &context,\n> > RkISP1Params *params)\n> > {\n> > uint32_t ct = context.activeState.awb.temperatureK;\n> \n> On frame zero - what is this initialised to ? Is it something sane? Is\n> it '0' ? And mostly is it something that won't badly affect the code below?\n> \n> If the LSC interpolations get clamped to sane ranges, then I would be\n> happy with this - but I can't tell from the context of this patch alone,\n> however I do suspect that's the case so:\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> > - if (std::abs(static_cast(ct) - static_cast(lastAppliedCt_)) <\n> > + /*\n> > + * The ISP in the imx8mp using linux 6.12 has a high probability for a\n\ns/imx8mp/i.MX8MP/\ns/linux/Linux/\n\n> > + * stall when the lsc module isn't configured in the first frame.\n\ns/lsc/LSC/\n\nMaybe expand it to \"is configured after starting the ISP if it hasn't\npreviously been configured for the first frame\".\n\n> > + * Therefore ensure that a lsc table is written unconditionally at the\n\nDitto.\n\n> > + * first frame.\n> > + */\n> > + if (frame > 0 &&\n> > + std::abs(static_cast(ct) - static_cast(lastAppliedCt_)) <\n> > kColourTemperatureChangeThreshhold)\n> > return;\n> > +\n> > unsigned int quantizedCt;\n> > const Components &set = sets_.getInterpolated(ct, &quantizedCt);\n> > - if (lastAppliedQuantizedCt_ == quantizedCt)\n> > + if (frame > 0 && lastAppliedQuantizedCt_ == quantizedCt)\n> > return;\n> > \n> > auto config = params->block();","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 E8EC4C32F6\n\tfor ;\n\tTue, 17 Dec 2024 17:04:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17B2A67FF1;\n\tTue, 17 Dec 2024 18:04:55 +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 CD8F267FD3\n\tfor ;\n\tTue, 17 Dec 2024 18:04:53 +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 248194C7;\n\tTue, 17 Dec 2024 18:04:16 +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=\"AkinNxtn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734455056;\n\tbh=ZwF35wR77pifReSnBUExRJGkHO+nUouyE3azWnDtFl4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AkinNxtnhE0QiM6QO2I/AtE9TkzIKuVpc3ptDbIV5Zw+qGCwjgPdeG4XAhf7I5SKl\n\toYRcdxZ+lqBFX8IH8I8X8KbzbO6K7RpC+LV3nYqR0GTEHWtzkIr6yrbisaIC7/z/gb\n\tFL02FMzKhadzzEsW0j3OuSdwBirCMzU8onb9zPc8=","Date":"Tue, 17 Dec 2024 19:04:51 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH] ipa: rkisp1: Initialize lsc tables on first frame","Message-ID":"<20241217170451.GL20432@pendragon.ideasonboard.com>","References":"<20241216153344.201302-1-stefan.klug@ideasonboard.com>\n\t<173445297887.32744.14806654289831116215@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173445297887.32744.14806654289831116215@ping.linuxembedded.co.uk>","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\" "}}]