[{"id":15851,"web_url":"https://patchwork.libcamera.org/comment/15851/","msgid":"","date":"2021-03-24T07:12:08","subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for the patch !\n\nOn 18/03/2021 11:39, Jacopo Mondi wrote:\n> Apply the last three hunks of commit 243d134 (\"Fix some bug for some\n> resolutions\") from https://github.com/intel/intel-ipu3-pipecfg.git\n> to the BSD calculation procedure.\n\ns/BSD/BDS ?\n\n> The BSD calculation is now perfomed by scaling both width and height,\n> and repeated by scaling width first.\n\ns/BSD/BDS ?\n\n> Signed-off-by: Jacopo Mondi \nTested-by: Jean-Michel Hautbois \nReviewed-by: Jean-Michel Hautbois \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++-----\n> 1 file changed, 25 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index d5cf05b0c421..4a1b0a9528f2 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -394,19 +394,39 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> \tconst Size &in = pipe->input;\n> \tSize gdc = calculateGDC(pipe);\n> \n> -\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> -\tunsigned int ifHeight = in.height;\n> -\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> \tfloat bdsSF = static_cast(in.width) / gdc.width;\n> \tfloat sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> \n> +\t/* Populate the configurations vector by scaling widht and height. */\n> +\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> +\tunsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> +\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> +\tunsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> \twhile (ifWidth >= minIfWidth) {\n> -\t\tSize iif{ ifWidth, ifHeight };\n> -\t\tcalculateBDS(pipe, iif, gdc, sf);\n> +\t\twhile (ifHeight >= minIfHeight) {\n> +\t\t\tSize iif{ ifWidth, ifHeight };\n> +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> +\t\t\tifHeight -= IF_ALIGN_H;\n> +\t\t}\n> \n> \t\tifWidth -= IF_ALIGN_W;\n> \t}\n> \n> +\t/* Repeat search by scaling width first. */\n> +\tifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> +\tifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> +\tminIfWidth = in.width - IF_CROP_MAX_W;\n> +\tminIfHeight = in.height - IF_CROP_MAX_H;\n> +\twhile (ifHeight >= minIfHeight) {\n> +\t\twhile (ifWidth >= minIfWidth) {\n> +\t\t\tSize iif{ ifWidth, ifHeight };\n> +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> +\t\t\tifWidth -= IF_ALIGN_W;\n> +\t\t}\n> +\n> +\t\tifHeight -= IF_ALIGN_H;\n> +\t}\n> +\n> \tif (pipeConfigs.size() == 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration\";\n> \t\treturn {};\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 D1364C32E5\n\tfor ;\n\tWed, 24 Mar 2021 07:12:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40A2368D68;\n\tWed, 24 Mar 2021 08:12:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82B0268D47\n\tfor ;\n\tWed, 24 Mar 2021 08:12:09 +0100 (CET)","from [IPv6:2a01:e0a:169:7140:21bd:98cc:c21e:1364] (unknown\n\t[IPv6:2a01:e0a:169:7140:21bd:98cc:c21e:1364])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B770580;\n\tWed, 24 Mar 2021 08:12:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rcHKKuIt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616569929;\n\tbh=LrqV0mhcWit/wc6QVoO3oHxoVg5JA2rL2OmsupM5YSw=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=rcHKKuIti2rgG7Shza0MfcqdjKkBxyjYklmDu9aXOk26cTmkuwjoHlfPnKP4XWnZR\n\tfNHSm027XcCT4jsv4YSyoOO1XGwBd3PB/lYOGWDIVK5xKjvXlyEFZ6DjHg5NLot8rs\n\tqwr9ivwzR8ZuK23H54Lb2RBNWndcb6It/a2xkbdQ=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-2-jacopo@jmondi.org>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Wed, 24 Mar 2021 08:12:08 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210318103941.18837-2-jacopo@jmondi.org>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16191,"web_url":"https://patchwork.libcamera.org/comment/16191/","msgid":"","date":"2021-04-12T23:18:01","subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Mar 18, 2021 at 11:39:35AM +0100, Jacopo Mondi wrote:\n> Apply the last three hunks of commit 243d134 (\"Fix some bug for some\n> resolutions\") from https://github.com/intel/intel-ipu3-pipecfg.git\n> to the BSD calculation procedure.\n\n\ns/BSD/BDS/\n\nand below too.\n\n> The BSD calculation is now perfomed by scaling both width and height,\n> and repeated by scaling width first.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++-----\n> 1 file changed, 25 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index d5cf05b0c421..4a1b0a9528f2 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -394,19 +394,39 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> \tconst Size &in = pipe->input;\n> \tSize gdc = calculateGDC(pipe);\n> \n> -\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> -\tunsigned int ifHeight = in.height;\n> -\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> \tfloat bdsSF = static_cast(in.width) / gdc.width;\n> \tfloat sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> \n> +\t/* Populate the configurations vector by scaling widht and height. */\n\ns/widht/width/\n\n> +\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> +\tunsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> +\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> +\tunsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> \twhile (ifWidth >= minIfWidth) {\n> -\t\tSize iif{ ifWidth, ifHeight };\n> -\t\tcalculateBDS(pipe, iif, gdc, sf);\n> +\t\twhile (ifHeight >= minIfHeight) {\n> +\t\t\tSize iif{ ifWidth, ifHeight };\n> +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> +\t\t\tifHeight -= IF_ALIGN_H;\n> +\t\t}\n> \n> \t\tifWidth -= IF_ALIGN_W;\n> \t}\n\nWhile this seems to replicate the python code mentioned in the commit\nmessage, the logic seems very strange to me. In the first iteration of\nthe outer loop, the inner loop will iterate over all ifHeight values\nlarger than the minimum. The inner loop will exit with ifHeight having\nan invalid value. Then the second iteration of the outer loop will try\nthe next ifWidth value, but the inner loop will be skipped, as ifHeight\nwill be < minIfHeight already.\n\nI'll reiterate my previous concern: if we merely copy the python code\nwithout understanding the logic, this will always remain broken.\n\n> \n> +\t/* Repeat search by scaling width first. */\n> +\tifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> +\tifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> +\tminIfWidth = in.width - IF_CROP_MAX_W;\n> +\tminIfHeight = in.height - IF_CROP_MAX_H;\n> +\twhile (ifHeight >= minIfHeight) {\n> +\t\twhile (ifWidth >= minIfWidth) {\n> +\t\t\tSize iif{ ifWidth, ifHeight };\n> +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> +\t\t\tifWidth -= IF_ALIGN_W;\n> +\t\t}\n> +\n> +\t\tifHeight -= IF_ALIGN_H;\n> +\t}\n> +\n> \tif (pipeConfigs.size() == 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration\";\n> \t\treturn {};","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 64065BD224\n\tfor ;\n\tMon, 12 Apr 2021 23:18:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7922687EC;\n\tTue, 13 Apr 2021 01:18:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 227CC605AE\n\tfor ;\n\tTue, 13 Apr 2021 01:18:51 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E22889A;\n\tTue, 13 Apr 2021 01:18:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"e0cMW/XE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618269530;\n\tbh=Kd2AR2C2v4KJLMHFUyArL5Udyrkgjd4v8gW5QMj7nZc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e0cMW/XEnwAl23tjvixyCVSUFi6waorKunScoW9z2Lozs3Iig+sOBihVHoB/2DCAi\n\tpIps0BvG6aEpDDvj81oivO9OifGAC+sNfgBQ6eCJIm7n6Ukzk3e/4Enpyo9QsCIbrB\n\tglS9NjvOmR6PzH/YxioiSFvkUTuRXOeTPPD0dWpw=","Date":"Tue, 13 Apr 2021 02:18:01 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210318103941.18837-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16221,"web_url":"https://patchwork.libcamera.org/comment/16221/","msgid":"<20210413074621.qpem74ap6efxdva4@uno.localdomain>","date":"2021-04-13T07:46:21","subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Tue, Apr 13, 2021 at 02:18:01AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Mar 18, 2021 at 11:39:35AM +0100, Jacopo Mondi wrote:\n> > Apply the last three hunks of commit 243d134 (\"Fix some bug for some\n> > resolutions\") from https://github.com/intel/intel-ipu3-pipecfg.git\n> > to the BSD calculation procedure.\n>\n>\n> s/BSD/BDS/\n>\n> and below too.\n>\n> > The BSD calculation is now perfomed by scaling both width and height,\n> > and repeated by scaling width first.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++-----\n> > 1 file changed, 25 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index d5cf05b0c421..4a1b0a9528f2 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -394,19 +394,39 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> > \tconst Size &in = pipe->input;\n> > \tSize gdc = calculateGDC(pipe);\n> >\n> > -\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > -\tunsigned int ifHeight = in.height;\n> > -\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > \tfloat bdsSF = static_cast(in.width) / gdc.width;\n> > \tfloat sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> >\n> > +\t/* Populate the configurations vector by scaling widht and height. */\n>\n> s/widht/width/\n>\n> > +\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > +\tunsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > +\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > +\tunsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> > \twhile (ifWidth >= minIfWidth) {\n> > -\t\tSize iif{ ifWidth, ifHeight };\n> > -\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > +\t\twhile (ifHeight >= minIfHeight) {\n> > +\t\t\tSize iif{ ifWidth, ifHeight };\n> > +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > +\t\t\tifHeight -= IF_ALIGN_H;\n> > +\t\t}\n> >\n> > \t\tifWidth -= IF_ALIGN_W;\n> > \t}\n>\n> While this seems to replicate the python code mentioned in the commit\n> message, the logic seems very strange to me. In the first iteration of\n> the outer loop, the inner loop will iterate over all ifHeight values\n> larger than the minimum. The inner loop will exit with ifHeight having\n> an invalid value. Then the second iteration of the outer loop will try\n> the next ifWidth value, but the inner loop will be skipped, as ifHeight\n> will be < minIfHeight already.\n>\n> I'll reiterate my previous concern: if we merely copy the python code\n> without understanding the logic, this will always remain broken.\n>\n\nI'm sorry but all I can do, without any documentation and with just\nthe python script as information source about the ImgU pipe\nconfiguration procedure is to make sure we stay in sync with the most\nrecent script version and that the two behaves the same. This part\nwould have to be rewritten as it is embarassing, but I'm a bit\nskeptical about distancing ourselves from the tool, as backporting\nfixes will become very hard.\n\nI'm fine dropping this series if there's a plan to re-implement the\npipe configuration.\n\n> >\n> > +\t/* Repeat search by scaling width first. */\n> > +\tifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > +\tifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > +\tminIfWidth = in.width - IF_CROP_MAX_W;\n> > +\tminIfHeight = in.height - IF_CROP_MAX_H;\n> > +\twhile (ifHeight >= minIfHeight) {\n> > +\t\twhile (ifWidth >= minIfWidth) {\n> > +\t\t\tSize iif{ ifWidth, ifHeight };\n> > +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > +\t\t\tifWidth -= IF_ALIGN_W;\n> > +\t\t}\n> > +\n> > +\t\tifHeight -= IF_ALIGN_H;\n> > +\t}\n> > +\n> > \tif (pipeConfigs.size() == 0) {\n> > \t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration\";\n> > \t\treturn {};\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 20783BD1F6\n\tfor ;\n\tTue, 13 Apr 2021 07:45:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C554687FE;\n\tTue, 13 Apr 2021 09:45:44 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 689AF687F3\n\tfor ;\n\tTue, 13 Apr 2021 09:45:43 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id DF39910000C;\n\tTue, 13 Apr 2021 07:45:42 +0000 (UTC)"],"Date":"Tue, 13 Apr 2021 09:46:21 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210413074621.qpem74ap6efxdva4@uno.localdomain>","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16457,"web_url":"https://patchwork.libcamera.org/comment/16457/","msgid":"","date":"2021-04-21T13:46:28","subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Apr 13, 2021 at 09:46:21AM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 13, 2021 at 02:18:01AM +0300, Laurent Pinchart wrote:\n> > On Thu, Mar 18, 2021 at 11:39:35AM +0100, Jacopo Mondi wrote:\n> > > Apply the last three hunks of commit 243d134 (\"Fix some bug for some\n> > > resolutions\") from https://github.com/intel/intel-ipu3-pipecfg.git\n> > > to the BSD calculation procedure.\n> >\n> > s/BSD/BDS/\n> >\n> > and below too.\n> >\n> > > The BSD calculation is now perfomed by scaling both width and height,\n> > > and repeated by scaling width first.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++-----\n> > > 1 file changed, 25 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > index d5cf05b0c421..4a1b0a9528f2 100644\n> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > @@ -394,19 +394,39 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> > > \tconst Size &in = pipe->input;\n> > > \tSize gdc = calculateGDC(pipe);\n> > >\n> > > -\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > -\tunsigned int ifHeight = in.height;\n> > > -\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > \tfloat bdsSF = static_cast(in.width) / gdc.width;\n> > > \tfloat sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> > >\n> > > +\t/* Populate the configurations vector by scaling widht and height. */\n> >\n> > s/widht/width/\n> >\n> > > +\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > +\tunsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > +\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > +\tunsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> > > \twhile (ifWidth >= minIfWidth) {\n> > > -\t\tSize iif{ ifWidth, ifHeight };\n> > > -\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > > +\t\twhile (ifHeight >= minIfHeight) {\n> > > +\t\t\tSize iif{ ifWidth, ifHeight };\n> > > +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > > +\t\t\tifHeight -= IF_ALIGN_H;\n> > > +\t\t}\n> > >\n> > > \t\tifWidth -= IF_ALIGN_W;\n> > > \t}\n> >\n> > While this seems to replicate the python code mentioned in the commit\n> > message, the logic seems very strange to me. In the first iteration of\n> > the outer loop, the inner loop will iterate over all ifHeight values\n> > larger than the minimum. The inner loop will exit with ifHeight having\n> > an invalid value. Then the second iteration of the outer loop will try\n> > the next ifWidth value, but the inner loop will be skipped, as ifHeight\n> > will be < minIfHeight already.\n> >\n> > I'll reiterate my previous concern: if we merely copy the python code\n> > without understanding the logic, this will always remain broken.\n> \n> I'm sorry but all I can do, without any documentation and with just\n> the python script as information source about the ImgU pipe\n> configuration procedure is to make sure we stay in sync with the most\n> recent script version and that the two behaves the same. This part\n> would have to be rewritten as it is embarassing, but I'm a bit\n> skeptical about distancing ourselves from the tool, as backporting\n> fixes will become very hard.\n\nWe have various tools at our disposal, one of them being\nreverse-engineering :-) I don't agree that there's nothing we can do,\nbut I agree it will be a significant effort.\n\n> I'm fine dropping this series if there's a plan to re-implement the\n> pipe configuration.\n\nI don't think there's a need to drop it. As commented separately, the\ncode isn't significantly worse, and if this fixes issues and doesn't\ncause regressions, it's OK for now as we have other priorities than\nrewriting this implementation.\n\n> > > +\t/* Repeat search by scaling width first. */\n> > > +\tifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > +\tifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > +\tminIfWidth = in.width - IF_CROP_MAX_W;\n> > > +\tminIfHeight = in.height - IF_CROP_MAX_H;\n> > > +\twhile (ifHeight >= minIfHeight) {\n> > > +\t\twhile (ifWidth >= minIfWidth) {\n> > > +\t\t\tSize iif{ ifWidth, ifHeight };\n> > > +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > > +\t\t\tifWidth -= IF_ALIGN_W;\n> > > +\t\t}\n> > > +\n> > > +\t\tifHeight -= IF_ALIGN_H;\n> > > +\t}\n> > > +\n> > > \tif (pipeConfigs.size() == 0) {\n> > > \t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration\";\n> > > \t\treturn {};","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 2BF0FBDB15\n\tfor ;\n\tWed, 21 Apr 2021 13:46:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A579B6884A;\n\tWed, 21 Apr 2021 15:46:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3810568835\n\tfor ;\n\tWed, 21 Apr 2021 15:46:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A81BB4AE;\n\tWed, 21 Apr 2021 15:46:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uFrIHj3O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619012792;\n\tbh=mVBPeXUC9nnIEebhvqzAvd08zz/Mowe9geMy49XLLKQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uFrIHj3OfIGhCURFgYRJDvCi2Jvq0vuRzWLHGxpdwYjERYRoaYu8hkAL/MBqSgQaa\n\tW4hAI1byQuLH/CJ1RXhVgwTGFJD0k3PaWIZBPgAGhjOikd6dz8gJi9Na/qz3PRhPWy\n\t1GghJ4JuBbbXxgxLmNcmm7GyoXIxVqxo2VO2TZkg=","Date":"Wed, 21 Apr 2021 16:46:28 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-2-jacopo@jmondi.org>\n\t\n\t<20210413074621.qpem74ap6efxdva4@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210413074621.qpem74ap6efxdva4@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16725,"web_url":"https://patchwork.libcamera.org/comment/16725/","msgid":"<20210501093720.m6tkbdp2bni6y6ba@uno.localdomain>","date":"2021-05-01T09:37:20","subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 13, 2021 at 02:18:01AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Mar 18, 2021 at 11:39:35AM +0100, Jacopo Mondi wrote:\n> > Apply the last three hunks of commit 243d134 (\"Fix some bug for some\n> > resolutions\") from https://github.com/intel/intel-ipu3-pipecfg.git\n> > to the BSD calculation procedure.\n>\n>\n> s/BSD/BDS/\n>\n> and below too.\n>\n> > The BSD calculation is now perfomed by scaling both width and height,\n> > and repeated by scaling width first.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++-----\n> > 1 file changed, 25 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index d5cf05b0c421..4a1b0a9528f2 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -394,19 +394,39 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> > \tconst Size &in = pipe->input;\n> > \tSize gdc = calculateGDC(pipe);\n> >\n> > -\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > -\tunsigned int ifHeight = in.height;\n> > -\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > \tfloat bdsSF = static_cast(in.width) / gdc.width;\n> > \tfloat sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> >\n> > +\t/* Populate the configurations vector by scaling widht and height. */\n>\n> s/widht/width/\n>\n> > +\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > +\tunsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > +\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > +\tunsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> > \twhile (ifWidth >= minIfWidth) {\n> > -\t\tSize iif{ ifWidth, ifHeight };\n> > -\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > +\t\twhile (ifHeight >= minIfHeight) {\n> > +\t\t\tSize iif{ ifWidth, ifHeight };\n> > +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > +\t\t\tifHeight -= IF_ALIGN_H;\n> > +\t\t}\n> >\n> > \t\tifWidth -= IF_ALIGN_W;\n> > \t}\n>\n> While this seems to replicate the python code mentioned in the commit\n> message, the logic seems very strange to me. In the first iteration of\n> the outer loop, the inner loop will iterate over all ifHeight values\n> larger than the minimum. The inner loop will exit with ifHeight having\n> an invalid value. Then the second iteration of the outer loop will try\n> the next ifWidth value, but the inner loop will be skipped, as ifHeight\n> will be < minIfHeight already.\n\nIndeed, ifHeight should be re-initialized\n\nI've opened an issue on the script repository to report this\nhttps://github.com/intel/intel-ipu3-pipecfg/issues/2\n\n>\n> I'll reiterate my previous concern: if we merely copy the python code\n> without understanding the logic, this will always remain broken.\n>\n> >\n> > +\t/* Repeat search by scaling width first. */\n> > +\tifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > +\tifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > +\tminIfWidth = in.width - IF_CROP_MAX_W;\n> > +\tminIfHeight = in.height - IF_CROP_MAX_H;\n> > +\twhile (ifHeight >= minIfHeight) {\n> > +\t\twhile (ifWidth >= minIfWidth) {\n> > +\t\t\tSize iif{ ifWidth, ifHeight };\n> > +\t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > +\t\t\tifWidth -= IF_ALIGN_W;\n> > +\t\t}\n> > +\n> > +\t\tifHeight -= IF_ALIGN_H;\n> > +\t}\n> > +\n> > \tif (pipeConfigs.size() == 0) {\n> > \t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration\";\n> > \t\treturn {};\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 5D8E1BDE6D\n\tfor ;\n\tSat, 1 May 2021 09:36:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C984A68901;\n\tSat, 1 May 2021 11:36:39 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6277D688AE\n\tfor ;\n\tSat, 1 May 2021 11:36:38 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id CE37760003;\n\tSat, 1 May 2021 09:36:37 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Sat, 1 May 2021 11:37:20 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210501093720.m6tkbdp2bni6y6ba@uno.localdomain>","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS\n\tcalculation process","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]