[{"id":15854,"web_url":"https://patchwork.libcamera.org/comment/15854/","msgid":"<4d2a59ab-62f2-fa80-2084-381b4ffbdd4c@ideasonboard.com>","date":"2021-03-24T07:15:40","subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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 to calculateBDSHeight() function the first hunk of commit 243d134\n> (\"Fix some bug for some resolutions\") from\n> https://github.com/intel/intel-ipu3-pipecfg.git.\n> \n> The condition for the computed IF rectangle height to be matched\n> against the desired alignment now makes sure that it is included\n> in the minimum and maximum acceptable values.\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 | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 11bb7cb95084..89a01bddbed2 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\testIFHeight = std::clamp(estIFHeight, minIFHeight, iif.height);\n> \n> \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> -\t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n> +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> \n> \t\t\tbdsHeight = ifHeight / bdsSF;\n> \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\n> @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\t}\n> \n> \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> -\t\twhile (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {\n> +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> \n> \t\t\tbdsHeight = ifHeight / bdsSF;\n> \t\t\tif (std::fmod(bdsHeight, 1.0) == 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 27A49C32E7\n\tfor ;\n\tWed, 24 Mar 2021 07:15:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DEF6C68D63;\n\tWed, 24 Mar 2021 08:15:42 +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 09A7E68D47\n\tfor ;\n\tWed, 24 Mar 2021 08:15:41 +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 B1521580;\n\tWed, 24 Mar 2021 08:15:40 +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=\"Y1Qrou3m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616570140;\n\tbh=HaEE1t/Pn81+m5Ew2z5oD/kXjqRLF1HVo2K8dFQZMUI=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Y1Qrou3m25xiXttBtM7fKHu2ZbWFR0fn1HdkTEzXlIIAhzTHjsNReLPLdH5GT9OMA\n\tNuZZVGa8xZXm5POQj4u1+/Jz2ItkjMRHBnQGbBvyjjo2wJYNlWcKVixvjoQAoLIO+W\n\tDhfBsUoZZQpLTkJ7kA1vaC+jbCwOuoP5v+8v3qUU=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-5-jacopo@jmondi.org>","From":"Jean-Michel Hautbois ","Message-ID":"<4d2a59ab-62f2-fa80-2084-381b4ffbdd4c@ideasonboard.com>","Date":"Wed, 24 Mar 2021 08:15:40 +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-5-jacopo@jmondi.org>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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":16195,"web_url":"https://patchwork.libcamera.org/comment/16195/","msgid":"","date":"2021-04-13T00:14:43","subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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:38AM +0100, Jacopo Mondi wrote:\n> Apply to calculateBDSHeight() function the first hunk of commit 243d134\n> (\"Fix some bug for some resolutions\") from\n> https://github.com/intel/intel-ipu3-pipecfg.git.\n> \n> The condition for the computed IF rectangle height to be matched\n> against the desired alignment now makes sure that it is included\n> in the minimum and maximum acceptable values.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 11bb7cb95084..89a01bddbed2 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\testIFHeight = std::clamp(estIFHeight, minIFHeight, iif.height);\n> \n> \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> -\t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n> +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n\niif.height isn't changed in the loop, and ifHeight is only decreased.\nThe new condition can either be false on the first iteration, in which\ncase the loop will be skipped completely, or it will always be true and\nwon't have an effect.\n\n> +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> \n> \t\t\tbdsHeight = ifHeight / bdsSF;\n> \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\n> @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\t}\n> \n> \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> -\t\twhile (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {\n> +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n\nIf the condition was false on the first iteration of the previous loop,\nif will still be false here, and both loops will be skipped. There's\nsomething fishy in this code. It doesn't seem significantly worse after\nthis change though.\n\n> +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> \n> \t\t\tbdsHeight = ifHeight / bdsSF;\n> \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {","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 53EB2BD1F6\n\tfor ;\n\tTue, 13 Apr 2021 00:15:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA14D687F4;\n\tTue, 13 Apr 2021 02:15: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 38443605AE\n\tfor ;\n\tTue, 13 Apr 2021 02:15: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 AB2496F2;\n\tTue, 13 Apr 2021 02:15: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=\"COyBGKM3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618272932;\n\tbh=x+hrjookKq2YXVTgggrl2prehK3kguTuiV1vWWeK6dk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=COyBGKM30fkhAXOYnwkbt079iWaiaHnXvkZpcnMfndJJFYgEwFhZ1aNARXmaZ9x2P\n\t6ymHPsSwk2u1T+Bw+j9pbHii2spebUOdfy4Xwwuy5tlNG31zui0aTRi9qr6r/5wsae\n\tOAzBz4+9Fa2ulwyk/YDovs7kq+8sCk4JjAiMbN5U=","Date":"Tue, 13 Apr 2021 03:14:43 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210318103941.18837-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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":16454,"web_url":"https://patchwork.libcamera.org/comment/16454/","msgid":"<20210421130118.mh2y3dbz7ujy2khp@uno.localdomain>","date":"2021-04-21T13:01:18","subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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 03:14:43AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Mar 18, 2021 at 11:39:38AM +0100, Jacopo Mondi wrote:\n> > Apply to calculateBDSHeight() function the first hunk of commit 243d134\n> > (\"Fix some bug for some resolutions\") from\n> > https://github.com/intel/intel-ipu3-pipecfg.git.\n> >\n> > The condition for the computed IF rectangle height to be matched\n> > against the desired alignment now makes sure that it is included\n> > in the minimum and maximum acceptable values.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++--\n> > 1 file changed, 4 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 11bb7cb95084..89a01bddbed2 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > \t\testIFHeight = std::clamp(estIFHeight, minIFHeight, iif.height);\n> >\n> > \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > -\t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n> > +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n>\n> iif.height isn't changed in the loop, and ifHeight is only decreased.\n> The new condition can either be false on the first iteration, in which\n> case the loop will be skipped completely, or it will always be true and\n> won't have an effect.\n>\n> > +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> >\n> > \t\t\tbdsHeight = ifHeight / bdsSF;\n> > \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\n> > @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > \t\t}\n> >\n> > \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > -\t\twhile (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {\n> > +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n>\n> If the condition was false on the first iteration of the previous loop,\n> if will still be false here, and both loops will be skipped. There's\n> something fishy in this code. It doesn't seem significantly worse after\n> this change though.\n>\n\nCorrect, but in this second loop ifHeight is increased, so the check\nis required. I agree in the first loop it could have been a condition\nexternal to the while() but it would be one indentation level that can\nbe skipped imho.\n\nAgain, deviating from the script is risky in terms of eventual backporting\n\n> > +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> >\n> > \t\t\tbdsHeight = ifHeight / bdsSF;\n> > \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\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 D1ED3BDB15\n\tfor ;\n\tWed, 21 Apr 2021 13:00:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50FBB6884A;\n\tWed, 21 Apr 2021 15:00:39 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6998368835\n\tfor ;\n\tWed, 21 Apr 2021 15:00:38 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id B307524000B;\n\tWed, 21 Apr 2021 13:00:37 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 21 Apr 2021 15:01:18 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210421130118.mh2y3dbz7ujy2khp@uno.localdomain>","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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":16456,"web_url":"https://patchwork.libcamera.org/comment/16456/","msgid":"","date":"2021-04-21T13:42:53","subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Apr 21, 2021 at 03:01:18PM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 13, 2021 at 03:14:43AM +0300, Laurent Pinchart wrote:\n> > On Thu, Mar 18, 2021 at 11:39:38AM +0100, Jacopo Mondi wrote:\n> > > Apply to calculateBDSHeight() function the first hunk of commit 243d134\n> > > (\"Fix some bug for some resolutions\") from\n> > > https://github.com/intel/intel-ipu3-pipecfg.git.\n> > >\n> > > The condition for the computed IF rectangle height to be matched\n> > > against the desired alignment now makes sure that it is included\n> > > in the minimum and maximum acceptable values.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++--\n> > > 1 file changed, 4 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > index 11bb7cb95084..89a01bddbed2 100644\n> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > > \t\testIFHeight = std::clamp(estIFHeight, minIFHeight, iif.height);\n> > >\n> > > \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > > -\t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n> > > +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> >\n> > iif.height isn't changed in the loop, and ifHeight is only decreased.\n> > The new condition can either be false on the first iteration, in which\n> > case the loop will be skipped completely, or it will always be true and\n> > won't have an effect.\n> >\n> > > +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> > >\n> > > \t\t\tbdsHeight = ifHeight / bdsSF;\n> > > \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\n> > > @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > > \t\t}\n> > >\n> > > \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > > -\t\twhile (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {\n> > > +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> >\n> > If the condition was false on the first iteration of the previous loop,\n> > if will still be false here, and both loops will be skipped. There's\n> > something fishy in this code. It doesn't seem significantly worse after\n> > this change though.\n> >\n> \n> Correct, but in this second loop ifHeight is increased, so the check\n> is required. I agree in the first loop it could have been a condition\n> external to the while() but it would be one indentation level that can\n> be skipped imho.\n> \n> Again, deviating from the script is risky in terms of eventual backporting\n\nAs commented on patch 3/7, I don't think that's a big issue, we\nshouldn't duplicate the python code in the first place.\n\n> > > +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> > >\n> > > \t\t\tbdsHeight = ifHeight / bdsSF;\n> > > \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {","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 5364ABDB16\n\tfor ;\n\tWed, 21 Apr 2021 13:42:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1195868840;\n\tWed, 21 Apr 2021 15:42:59 +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 D94DF68835\n\tfor ;\n\tWed, 21 Apr 2021 15:42:57 +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 6D5F64AE;\n\tWed, 21 Apr 2021 15:42:57 +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=\"jttzfvA6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619012577;\n\tbh=e2mf0DgcFEuY1mvx+qYuWcumCcS0sxfRYgj38ziF3Uw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jttzfvA6JjAIUI6eix/ugFN1P/sjjMPnsPVVSuAK/s2ewD1Yb/HZVaZYPM0Hnleuo\n\tt8RbZDGYCxxd/g6wtaj4S3LMDF8YliTt3H2FmLjnaKKullzUqD2S9M3o65Zg8soTnN\n\tpGuYopYeIqOXfoX+hQCbjumjK5RSe5mFz/mDzkIQ=","Date":"Wed, 21 Apr 2021 16:42:53 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-5-jacopo@jmondi.org>\n\t\n\t<20210421130118.mh2y3dbz7ujy2khp@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421130118.mh2y3dbz7ujy2khp@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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":16726,"web_url":"https://patchwork.libcamera.org/comment/16726/","msgid":"<20210501094909.2htz2fxbr6w72vbb@uno.localdomain>","date":"2021-05-01T09:49:09","subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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 03:14:43AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Mar 18, 2021 at 11:39:38AM +0100, Jacopo Mondi wrote:\n> > Apply to calculateBDSHeight() function the first hunk of commit 243d134\n> > (\"Fix some bug for some resolutions\") from\n> > https://github.com/intel/intel-ipu3-pipecfg.git.\n> >\n> > The condition for the computed IF rectangle height to be matched\n> > against the desired alignment now makes sure that it is included\n> > in the minimum and maximum acceptable values.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++--\n> > 1 file changed, 4 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 11bb7cb95084..89a01bddbed2 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > \t\testIFHeight = std::clamp(estIFHeight, minIFHeight, iif.height);\n> >\n> > \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > -\t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n> > +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n>\n> iif.height isn't changed in the loop, and ifHeight is only decreased.\n> The new condition can either be false on the first iteration, in which\n> case the loop will be skipped completely, or it will always be true and\n> won't have an effect.\n>\n> > +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> >\n> > \t\t\tbdsHeight = ifHeight / bdsSF;\n> > \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\n> > @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > \t\t}\n> >\n> > \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > -\t\twhile (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {\n> > +\t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n>\n> If the condition was false on the first iteration of the previous loop,\n> if will still be false here, and both loops will be skipped. There's\n> something fishy in this code. It doesn't seem significantly worse after\n> this change though.\n\nYou know, I presume this might even be intended, even if it was\nprobably better coded as\n\n if (ifHeight > iif.height) {\n while (ifHeight >= minIFHeight &&\n ifHeight / bdsSF >= minBDSHeight) {\n\n ....\n ifHeight -= IF_ALIGN_H;\n }\n\n while (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n ifHeight / bdsSF >= minBDSHeight) {\n\n ....\n ifHeight += IF_ALIGN_H;\n\n }\n }\n\n>\n> > +\t\t ifHeight / bdsSF >= minBDSHeight) {\n> >\n> > \t\t\tbdsHeight = ifHeight / bdsSF;\n> > \t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\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 5F9B0BDE6A\n\tfor ;\n\tSat, 1 May 2021 09:48:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3BA8688AE;\n\tSat, 1 May 2021 11:48:28 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 35970688AE\n\tfor ;\n\tSat, 1 May 2021 11:48:27 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 99380FF803;\n\tSat, 1 May 2021 09:48:26 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Sat, 1 May 2021 11:49:09 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210501094909.2htz2fxbr6w72vbb@uno.localdomain>","References":"<20210318103941.18837-1-jacopo@jmondi.org>\n\t<20210318103941.18837-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF\n\theight selection","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\" "}}]