[{"id":16739,"web_url":"https://patchwork.libcamera.org/comment/16739/","msgid":"","date":"2021-05-03T10:56:00","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-05-03 11:26:59 +0200, 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 BDS calculation procedure.\n> \n> The BDS calculation is now perfomed by scaling both width and height,\n> and repeated by scaling width first.\n> \n> Tested-by: Jean-Michel Hautbois \n> Reviewed-by: Jean-Michel Hautbois \n> Signed-off-by: Jacopo Mondi \n\nAcked-by: Niklas Söderlund \n\n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++++++++++----\n> 1 file changed, 29 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index d5cf05b0c421..8373dc165a61 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -394,19 +394,43 @@ 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 width 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\t/*\n> +\t\t * \\todo This procedure is probably broken:\n> +\t\t * https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> +\t\t */\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> 2.31.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 47A82BDE78\n\tfor ;\n\tMon, 3 May 2021 10:56:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 05B5F688AE;\n\tMon, 3 May 2021 12:56:03 +0200 (CEST)","from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com\n\t[IPv6:2a00:1450:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9D5CA60511\n\tfor ;\n\tMon, 3 May 2021 12:56:01 +0200 (CEST)","by mail-ej1-x62f.google.com with SMTP id y7so7166187ejj.9\n\tfor ;\n\tMon, 03 May 2021 03:56:01 -0700 (PDT)","from localhost (p54ac5521.dip0.t-ipconnect.de. [84.172.85.33])\n\tby smtp.gmail.com with ESMTPSA id\n\tq16sm12474753edv.61.2021.05.03.03.56.00\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 03 May 2021 03:56:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"E7SZ1Rq8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=U+Jq2lg+SyGvgWSZcnKBttmnTHqwlJrcE9r6AHnElVg=;\n\tb=E7SZ1Rq864PADChJ9kMR5/SF0Z/ZxD4JzVIU7YM4rGIayvVa2xbdvaZ9oJKloZtZFe\n\tRVQyFKoOdD4v37gzFNH9vxR6XB7YnA6iYtgQhq5+hj213UVHeitWLgDc9sTogHC+EOZR\n\t9bIjMdrqIWJwn0MOuBgfEtp2cDESFYf99PCTmluThP4AxnHdVl+hxXMQ855ui9MjQb22\n\tA800e6QOsFRh+kcOSq10jrasjXHDdWl+9vXW2/vXeHuu+zU07EfcmGrFhKFBqI/fOUMj\n\t3x0/l4HONP4YuBA6n5Rj2Va2gQn6YruYUMZ7jsnn5ga/YzQVW5+mWM8gVM1wizBWt24I\n\tVTKA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=U+Jq2lg+SyGvgWSZcnKBttmnTHqwlJrcE9r6AHnElVg=;\n\tb=MDAgIHqG56zn5rqY4VjJn9OtP5bZzmVX+Nbrk9t+aJc/mEvfbjXzhnLu3roy/O/fzQ\n\tE14Z/MEQHe3mBjNbIwQizzn4aYw+Umx9u65/u1M7vnv2RCUPKyktaGfq6MbEcXUGqZwd\n\tbqYnfOughCjZfQ6sC6HNyznrGpPwfw3vb4mK8OCaUi4dUab5ipJowt4eGYdZzdpDEp2h\n\tOHnZXdQGfbcft03sGypNFSYSAHH/N2UB8UO+750iv2kbLFFXy656VWL41dPO/m7ZfcSQ\n\tV+rymq68xgJbZJeqmXAhTp/VotgtXAtE4KWutNRe2Ks3mkXdE8eSEw3TlGFc7NsqgYj7\n\t+JLQ==","X-Gm-Message-State":"AOAM532t6g6lJkddf6yXHpluOZuz14SjHGOUiGvnZ2Qi5+9U0/Edfizg\n\tEn+plTGZmm6jnPTcWuwA3M41DZ3/g9WADlZpUc8=","X-Google-Smtp-Source":"ABdhPJwy/zOAldcpcZKB+nUL1X7ofaOLbj9BSjkiUmBYugnX0driiTRutLZEs6WtWCOAU1/B1yGdmw==","X-Received":"by 2002:a17:906:e84:: with SMTP id\n\tp4mr15925511ejf.248.1620039361308; \n\tMon, 03 May 2021 03:56:01 -0700 (PDT)","Date":"Mon, 3 May 2021 12:56:00 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210503092705.15562-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16783,"web_url":"https://patchwork.libcamera.org/comment/16783/","msgid":"","date":"2021-05-06T05:34:30","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the patch.\n\nOn Mon, May 3, 2021 at 7:56 PM Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-05-03 11:26:59 +0200, 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 BDS calculation procedure.\n> >\n> > The BDS calculation is now perfomed by scaling both width and height,\n> > and repeated by scaling width first.\n> >\n> > Tested-by: Jean-Michel Hautbois \n> > Reviewed-by: Jean-Michel Hautbois \n> > Signed-off-by: Jacopo Mondi \n>\n> Acked-by: Niklas Söderlund \n>\n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++++++++++----\n> > 1 file changed, 29 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index d5cf05b0c421..8373dc165a61 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -394,19 +394,43 @@ ImgUDevice::PipeConfig\n> ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> > const Size &in = pipe->input;\n> > Size gdc = calculateGDC(pipe);\n> >\n> > - unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > - unsigned int ifHeight = in.height;\n> > - unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > float bdsSF = static_cast(in.width) / gdc.width;\n> > float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> >\n> > + /* Populate the configurations vector by scaling width and height.\n> */\n> > + unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > + unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n>\n\nShould we std::max(0u, in.width - IF_CROP_MAX_W)?\nIf in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes\nvery large.\nDitto to height.\n\n\n> > while (ifWidth >= minIfWidth) {\n> > - Size iif{ ifWidth, ifHeight };\n> > - calculateBDS(pipe, iif, gdc, sf);\n> > + while (ifHeight >= minIfHeight) {\n> > + Size iif{ ifWidth, ifHeight };\n> > + calculateBDS(pipe, iif, gdc, sf);\n> > + ifHeight -= IF_ALIGN_H;\n> > + }\n> >\n> > ifWidth -= IF_ALIGN_W;\n> > }\n> >\n> > + /* Repeat search by scaling width first. */\n> > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > + minIfWidth = in.width - IF_CROP_MAX_W;\n> > + minIfHeight = in.height - IF_CROP_MAX_H;\n> > + while (ifHeight >= minIfHeight) {\n> > + /*\n> > + * \\todo This procedure is probably broken:\n> > + * https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> > + */\n> > + while (ifWidth >= minIfWidth) {\n> > + Size iif{ ifWidth, ifHeight };\n> > + calculateBDS(pipe, iif, gdc, sf);\n> > + ifWidth -= IF_ALIGN_W;\n> > + }\n> > +\n> > + ifHeight -= IF_ALIGN_H;\n> > + }\n> > +\n> > if (pipeConfigs.size() == 0) {\n> > LOG(IPU3, Error) << \"Failed to calculate pipe\n> configuration\";\n> > return {};\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 02EB1BDE7D\n\tfor ;\n\tThu, 6 May 2021 05:34:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FCA768911;\n\tThu, 6 May 2021 07:34:41 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B339A688E5\n\tfor ;\n\tThu, 6 May 2021 07:34:40 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id h10so4656829edt.13\n\tfor ;\n\tWed, 05 May 2021 22:34:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"MAwL3vjp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ry4yLGZVvfVVFRshhuehrOyqVtE+m20Ud8vqIEzEayc=;\n\tb=MAwL3vjp45H/nksyjLzr5lqbje5iZ3lAgbV8UbRbmqc9Z8Su3KrcgK504sV8yjQUzo\n\tdH409mN6/AjnPiRx3RjqVDsNZkqbL27xuffy5dqPImp9++0zMg9z1HcRQZSOuTQ46xm9\n\tvOZYMVgG7w0goa0A/uCk+qtsHMbwptt3MIvd8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ry4yLGZVvfVVFRshhuehrOyqVtE+m20Ud8vqIEzEayc=;\n\tb=BgrCghESY7Aln3sguTG3vK8vZtO1ZPHVmnWQpdcK32GV33W44mofFryUxVEiaCUd+G\n\tg+q7u1C/qUX5g0hnO6zSARiBUZIjTzVLKsiO1mFuTpE17u06DjVj3HemAYvKqLIPZM1F\n\tszHHYrG96mFnKAH+tOUO05mgJuLgrROjkt1t0q4O247o4v6fZKMk1F1KZPTGZIfygzsW\n\tAzfjYlMnO2lQpGAafst3kSyiX7SzSh+IsV9jTIM0U3SLY2uS5K9K5yyvUM7v3qdJsA6e\n\tFO76ly6K9+2k+/W9Xtb3U++NKgB8XbVx5TmxIkZ/8ZyA2UeMMVIYRN+uOoGCEXJ5fooX\n\tbwxw==","X-Gm-Message-State":"AOAM533vbvuVfEuA7yYCYVHma2/ZU46J4YM6dbKfBsbJmmMRaU3Bah0W\n\tKsSwdzV/J8lgbAfc5iXaWdZTcxCeXCen2Uj1xyyeW9qVFBjPLQ==","X-Google-Smtp-Source":"ABdhPJwT2SxYIZUEeQNUnJLq2mt6jKlaQ7DDQn2RVCA64NVT93XfsrjJUVLq6WJbtZnorPKHvkVXc6WxVFEVMoaChkw=","X-Received":"by 2002:a05:6402:50c6:: with SMTP id\n\th6mr2982597edb.327.1620279280394; \n\tWed, 05 May 2021 22:34:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 6 May 2021 14:34:30 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============1475561911457710177==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16795,"web_url":"https://patchwork.libcamera.org/comment/16795/","msgid":"<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>","date":"2021-05-06T08:15:59","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, May 06, 2021 at 02:34:30PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch.\n>\n> On Mon, May 3, 2021 at 7:56 PM Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n>\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2021-05-03 11:26:59 +0200, 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 BDS calculation procedure.\n> > >\n> > > The BDS calculation is now perfomed by scaling both width and height,\n> > > and repeated by scaling width first.\n> > >\n> > > Tested-by: Jean-Michel Hautbois \n> > > Reviewed-by: Jean-Michel Hautbois \n> > > Signed-off-by: Jacopo Mondi \n> >\n> > Acked-by: Niklas Söderlund \n> >\n> > > ---\n> > > src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++++++++++----\n> > > 1 file changed, 29 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > index d5cf05b0c421..8373dc165a61 100644\n> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > @@ -394,19 +394,43 @@ ImgUDevice::PipeConfig\n> > ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> > > const Size &in = pipe->input;\n> > > Size gdc = calculateGDC(pipe);\n> > >\n> > > - unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > - unsigned int ifHeight = in.height;\n> > > - unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > float bdsSF = static_cast(in.width) / gdc.width;\n> > > float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> > >\n> > > + /* Populate the configurations vector by scaling width and height.\n> > */\n> > > + unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > + unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> >\n>\n> Should we std::max(0u, in.width - IF_CROP_MAX_W)?\n\nI would be careful and not use 0, but probably 2 and 4 as it seems to be the\nvertical and horizontal alignments the ImgU requires\n\n> If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes\n> very large.\n> Ditto to height.\n\nLaurent had the same question, and we received patch in the past that\nwas probably related to an issue like this one, if the input size is\nsmaller than 540 (as IF_CROP_MAX_H=540).\nhttps://patchwork.libcamera.org/patch/11620/\n\nI've opened (yet) a new issue on the python script this code is based\non to have Intel's opinion on which is the min IF crop size\nhttps://github.com/intel/intel-ipu3-pipecfg/issues/3\n\nI'm thinking of fixing this issue, and the other ones reported during\nreview of this series (and reflected as issues on the script's github)\non top.\n\nI'm overly-concerned with the idea of deviating from the script. I've\nreceived several comments on how things could have been done better,\nbut I'm pushing back as I want this code to be as similar as possible\nto the intel's script, otherwise every 4 months when (I have to)\ncompare the two implementation my brain will melt even more than\nusual.\n\nResult: the code is of awful quality as the script is.\n\nI re-state the requirement from Intel to provide documentation on\nhow the ImgU sizes have to be computed instead of a buggy script\n(which I apreciate a lot don't get me wrong, I'm sure the author has\nput some of his free time to implement that as I'm quite sure nobody\nin there apreciate spending more company time on IPU3, but the result\nis a best effort implementation nobody is proud of).\n\nEven better if they could do directly in libcamera... a man can\ndream... So far we received close to 0 attention from them and surely\nwe have no leverage from here to convince them of the contrary :)\nMaybe you could push a little to have them consider us a little\nbit more ?\n\nThanks\n j\n\n>\n>\n> > > while (ifWidth >= minIfWidth) {\n> > > - Size iif{ ifWidth, ifHeight };\n> > > - calculateBDS(pipe, iif, gdc, sf);\n> > > + while (ifHeight >= minIfHeight) {\n> > > + Size iif{ ifWidth, ifHeight };\n> > > + calculateBDS(pipe, iif, gdc, sf);\n> > > + ifHeight -= IF_ALIGN_H;\n> > > + }\n> > >\n> > > ifWidth -= IF_ALIGN_W;\n> > > }\n> > >\n> > > + /* Repeat search by scaling width first. */\n> > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > + minIfWidth = in.width - IF_CROP_MAX_W;\n> > > + minIfHeight = in.height - IF_CROP_MAX_H;\n> > > + while (ifHeight >= minIfHeight) {\n> > > + /*\n> > > + * \\todo This procedure is probably broken:\n> > > + * https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> > > + */\n> > > + while (ifWidth >= minIfWidth) {\n> > > + Size iif{ ifWidth, ifHeight };\n> > > + calculateBDS(pipe, iif, gdc, sf);\n> > > + ifWidth -= IF_ALIGN_W;\n> > > + }\n> > > +\n> > > + ifHeight -= IF_ALIGN_H;\n> > > + }\n> > > +\n> > > if (pipeConfigs.size() == 0) {\n> > > LOG(IPU3, Error) << \"Failed to calculate pipe\n> > configuration\";\n> > > return {};\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 5C106BDE7F\n\tfor ;\n\tThu, 6 May 2021 08:15:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B57B16891A;\n\tThu, 6 May 2021 10:15:16 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC42568909\n\tfor ;\n\tThu, 6 May 2021 10:15:15 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id E00F6E000B;\n\tThu, 6 May 2021 08:15:14 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 6 May 2021 10:15:59 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16818,"web_url":"https://patchwork.libcamera.org/comment/16818/","msgid":"","date":"2021-05-07T03:13:14","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, May 6, 2021 at 5:15 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Thu, May 06, 2021 at 02:34:30PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the patch.\n> >\n> > On Mon, May 3, 2021 at 7:56 PM Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2021-05-03 11:26:59 +0200, 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 BDS calculation procedure.\n> > > >\n> > > > The BDS calculation is now perfomed by scaling both width and height,\n> > > > and repeated by scaling width first.\n> > > >\n> > > > Tested-by: Jean-Michel Hautbois <\n> jeanmichel.hautbois@ideasonboard.com>\n> > > > Reviewed-by: Jean-Michel Hautbois <\n> jeanmichel.hautbois@ideasonboard.com>\n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > > Acked-by: Niklas Söderlund \n> > >\n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/imgu.cpp | 34\n> ++++++++++++++++++++++++----\n> > > > 1 file changed, 29 insertions(+), 5 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > index d5cf05b0c421..8373dc165a61 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > @@ -394,19 +394,43 @@ ImgUDevice::PipeConfig\n> > > ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> > > > const Size &in = pipe->input;\n> > > > Size gdc = calculateGDC(pipe);\n> > > >\n> > > > - unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > > - unsigned int ifHeight = in.height;\n> > > > - unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > > float bdsSF = static_cast(in.width) / gdc.width;\n> > > > float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> > > >\n> > > > + /* Populate the configurations vector by scaling width and\n> height.\n> > > */\n> > > > + unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > > + unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> > >\n> >\n> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?\n>\n> I would be careful and not use 0, but probably 2 and 4 as it seems to be\n> the\n> vertical and horizontal alignments the ImgU requires\n>\n> > If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes\n> > very large.\n> > Ditto to height.\n>\n> Laurent had the same question, and we received patch in the past that\n> was probably related to an issue like this one, if the input size is\n> smaller than 540 (as IF_CROP_MAX_H=540).\n> https://patchwork.libcamera.org/patch/11620/\n>\n> I've opened (yet) a new issue on the python script this code is based\n> on to have Intel's opinion on which is the min IF crop size\n> https://github.com/intel/intel-ipu3-pipecfg/issues/3\n>\n>\nThe variable in the python script is (signed) integer.\nSo overflow doesn't happen.\nThe code may work even if width < IF_CROP_MAX_W (I don't check the code any\nmore than it)?\nOn the other hand, our code doesn't work because the following while-loop\ndoesn't loop..?\nI see your point. It is the best to set the minimum allowed value after we\nget their feedback.\nBut, hmm, shall we set it to IF_ALIGN_W as the temporary workaround to the\noverflow with todo comment?\nWell, since the code of causing the overflow is in top-of-tree, I don't\nmind merging this.\n\n-Hiro\n\nI'm thinking of fixing this issue, and the other ones reported during\n> review of this series (and reflected as issues on the script's github)\n> on top.\n>\n> I'm overly-concerned with the idea of deviating from the script. I've\n> received several comments on how things could have been done better,\n> but I'm pushing back as I want this code to be as similar as possible\n> to the intel's script, otherwise every 4 months when (I have to)\n> compare the two implementation my brain will melt even more than\n> usual.\n>\n> Result: the code is of awful quality as the script is.\n>\n> I re-state the requirement from Intel to provide documentation on\n> how the ImgU sizes have to be computed instead of a buggy script\n> (which I apreciate a lot don't get me wrong, I'm sure the author has\n> put some of his free time to implement that as I'm quite sure nobody\n> in there apreciate spending more company time on IPU3, but the result\n> is a best effort implementation nobody is proud of).\n>\n> Even better if they could do directly in libcamera... a man can\n> dream... So far we received close to 0 attention from them and surely\n> we have no leverage from here to convince them of the contrary :)\n> Maybe you could push a little to have them consider us a little\n> bit more ?\n>\n> Thanks\n> j\n>\n> >\n> >\n> > > > while (ifWidth >= minIfWidth) {\n> > > > - Size iif{ ifWidth, ifHeight };\n> > > > - calculateBDS(pipe, iif, gdc, sf);\n> > > > + while (ifHeight >= minIfHeight) {\n> > > > + Size iif{ ifWidth, ifHeight };\n> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > > > + ifHeight -= IF_ALIGN_H;\n> > > > + }\n> > > >\n> > > > ifWidth -= IF_ALIGN_W;\n> > > > }\n> > > >\n> > > > + /* Repeat search by scaling width first. */\n> > > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > > + minIfWidth = in.width - IF_CROP_MAX_W;\n> > > > + minIfHeight = in.height - IF_CROP_MAX_H;\n> > > > + while (ifHeight >= minIfHeight) {\n> > > > + /*\n> > > > + * \\todo This procedure is probably broken:\n> > > > + *\n> https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> > > > + */\n> > > > + while (ifWidth >= minIfWidth) {\n> > > > + Size iif{ ifWidth, ifHeight };\n> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > > > + ifWidth -= IF_ALIGN_W;\n> > > > + }\n> > > > +\n> > > > + ifHeight -= IF_ALIGN_H;\n> > > > + }\n> > > > +\n> > > > if (pipeConfigs.size() == 0) {\n> > > > LOG(IPU3, Error) << \"Failed to calculate pipe\n> > > configuration\";\n> > > > return {};\n> > > > --\n> > > > 2.31.1\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 DFD8CBF831\n\tfor ;\n\tFri, 7 May 2021 03:13:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CBD368918;\n\tFri, 7 May 2021 05:13:27 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3397B68909\n\tfor ;\n\tFri, 7 May 2021 05:13:26 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id bf4so8451900edb.11\n\tfor ;\n\tThu, 06 May 2021 20:13:26 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"W6ttNBfL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=6w4CI5kfUc4+CQsnazgVmDOFT7ODiP6iiknVI1/T0Sw=;\n\tb=W6ttNBfLdAniDlGgabTHb8oIZrsmo8t9+Ec8JtXwCruGsrx6dqGseaBaF7910Y0hTE\n\tzqw8IhjWzBdRlIGO27N+04CUbI1Nx9gA1HrrwSl2mH56RcSqKFPgR80yL2Ov5QmPrAHH\n\tEERE70LhNSsiRiJJ+Cf50Zn833hhwSFCcaLA4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=6w4CI5kfUc4+CQsnazgVmDOFT7ODiP6iiknVI1/T0Sw=;\n\tb=UZyoe0/lnJlTxfhps5OjJvgInI60kugec68pBYOO/KBFKYeUPfBNBmie89ji+rImlV\n\tGuM1oOszjL10CEYkuZFcq4GIYJSbYABQQh9cRWRwyCq8jFszbZUXRzqTSo7diKvce2Nk\n\tT1JP8E0aRN5ngkNY6P2rydvSvC385pJlFNuwJKm4W+tCRueuB8411OT0fTxM9YljFBcP\n\tDSJ4bvRMxIJYfVP7oqREOKE4mRYfjcJFeYrq9Fsz82nmI3GdvIvbo5W4TrkOeau1DpvK\n\tOm3I3KKrRGjh+Hk/+bmhfhd+PoSFCEwzofy77N22kTqgOkrg9p3/KuSPiWo787nzyAPA\n\tWmIA==","X-Gm-Message-State":"AOAM533gMAfpsT39hXW4E5pMGdx2hyk4T45wTbqLo1A178lVkEKspV/H\n\tRfDCVjIoqAFKyXHVZ8vEshKglM3oki3JHY3yjH5EYfkzbu61RQ==","X-Google-Smtp-Source":"ABdhPJxB3dor8go8sEZ4uJLJHVK630/S8zG3TknpmMem2w47NZx3GZnNud10Jys61kWJG9ieExaSnZmzsOGYqUvZSSU=","X-Received":"by 2002:a50:8e44:: with SMTP id 4mr8861162edx.244.1620357205824; \n\tThu, 06 May 2021 20:13:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>","In-Reply-To":"<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 7 May 2021 12:13:14 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============4501741205953342349==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16908,"web_url":"https://patchwork.libcamera.org/comment/16908/","msgid":"","date":"2021-05-12T04:23:14","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Fri, May 7, 2021 at 12:13 PM Hirokazu Honda wrote:\n\n> Hi Jacopo,\n>\n> On Thu, May 6, 2021 at 5:15 PM Jacopo Mondi wrote:\n>\n>> Hi Hiro,\n>>\n>> On Thu, May 06, 2021 at 02:34:30PM +0900, Hirokazu Honda wrote:\n>> > Hi Jacopo, thank you for the patch.\n>> >\n>> > On Mon, May 3, 2021 at 7:56 PM Niklas Söderlund <\n>> > niklas.soderlund@ragnatech.se> wrote:\n>> >\n>> > > Hi Jacopo,\n>> > >\n>> > > Thanks for your work.\n>> > >\n>> > > On 2021-05-03 11:26:59 +0200, 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 BDS calculation procedure.\n>> > > >\n>> > > > The BDS calculation is now perfomed by scaling both width and\n>> height,\n>> > > > and repeated by scaling width first.\n>> > > >\n>> > > > Tested-by: Jean-Michel Hautbois <\n>> jeanmichel.hautbois@ideasonboard.com>\n>> > > > Reviewed-by: Jean-Michel Hautbois <\n>> jeanmichel.hautbois@ideasonboard.com>\n>> > > > Signed-off-by: Jacopo Mondi \n>> > >\n>> > > Acked-by: Niklas Söderlund \n>> > >\n>> > > > ---\n>> > > > src/libcamera/pipeline/ipu3/imgu.cpp | 34\n>> ++++++++++++++++++++++++----\n>> > > > 1 file changed, 29 insertions(+), 5 deletions(-)\n>> > > >\n>> > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n>> > > b/src/libcamera/pipeline/ipu3/imgu.cpp\n>> > > > index d5cf05b0c421..8373dc165a61 100644\n>> > > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n>> > > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n>> > > > @@ -394,19 +394,43 @@ ImgUDevice::PipeConfig\n>> > > ImgUDevice::calculatePipeConfig(Pipe *pipe)\n>> > > > const Size &in = pipe->input;\n>> > > > Size gdc = calculateGDC(pipe);\n>> > > >\n>> > > > - unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n>> > > > - unsigned int ifHeight = in.height;\n>> > > > - unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n>> > > > float bdsSF = static_cast(in.width) / gdc.width;\n>> > > > float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n>> > > >\n>> > > > + /* Populate the configurations vector by scaling width and\n>> height.\n>> > > */\n>> > > > + unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n>> > > > + unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n>> > > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n>> > > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n>> > >\n>> >\n>> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?\n>>\n>> I would be careful and not use 0, but probably 2 and 4 as it seems to be\n>> the\n>> vertical and horizontal alignments the ImgU requires\n>>\n>> > If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes\n>> > very large.\n>> > Ditto to height.\n>>\n>> Laurent had the same question, and we received patch in the past that\n>> was probably related to an issue like this one, if the input size is\n>> smaller than 540 (as IF_CROP_MAX_H=540).\n>> https://patchwork.libcamera.org/patch/11620/\n>>\n>> I've opened (yet) a new issue on the python script this code is based\n>> on to have Intel's opinion on which is the min IF crop size\n>> https://github.com/intel/intel-ipu3-pipecfg/issues/3\n>>\n>>\n> The variable in the python script is (signed) integer.\n> So overflow doesn't happen.\n> The code may work even if width < IF_CROP_MAX_W (I don't check the code\n> any more than it)?\n> On the other hand, our code doesn't work because the following while-loop\n> doesn't loop..?\n> I see your point. It is the best to set the minimum allowed value after we\n> get their feedback.\n> But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround to the\n> overflow with todo comment?\n> Well, since the code of causing the overflow is in top-of-tree, I don't\n> mind merging this.\n>\n>\n\nPing. Can you respond to this before you merge this patch?\n\n\n> -Hiro\n>\n> I'm thinking of fixing this issue, and the other ones reported during\n>> review of this series (and reflected as issues on the script's github)\n>> on top.\n>>\n>> I'm overly-concerned with the idea of deviating from the script. I've\n>> received several comments on how things could have been done better,\n>> but I'm pushing back as I want this code to be as similar as possible\n>> to the intel's script, otherwise every 4 months when (I have to)\n>> compare the two implementation my brain will melt even more than\n>> usual.\n>>\n>> Result: the code is of awful quality as the script is.\n>>\n>> I re-state the requirement from Intel to provide documentation on\n>> how the ImgU sizes have to be computed instead of a buggy script\n>> (which I apreciate a lot don't get me wrong, I'm sure the author has\n>> put some of his free time to implement that as I'm quite sure nobody\n>> in there apreciate spending more company time on IPU3, but the result\n>> is a best effort implementation nobody is proud of).\n>>\n>> Even better if they could do directly in libcamera... a man can\n>> dream... So far we received close to 0 attention from them and surely\n>> we have no leverage from here to convince them of the contrary :)\n>> Maybe you could push a little to have them consider us a little\n>> bit more ?\n>>\n>> Thanks\n>> j\n>>\n>> >\n>> >\n>> > > > while (ifWidth >= minIfWidth) {\n>> > > > - Size iif{ ifWidth, ifHeight };\n>> > > > - calculateBDS(pipe, iif, gdc, sf);\n>> > > > + while (ifHeight >= minIfHeight) {\n>> > > > + Size iif{ ifWidth, ifHeight };\n>> > > > + calculateBDS(pipe, iif, gdc, sf);\n>> > > > + ifHeight -= IF_ALIGN_H;\n>> > > > + }\n>> > > >\n>> > > > ifWidth -= IF_ALIGN_W;\n>> > > > }\n>> > > >\n>> > > > + /* Repeat search by scaling width first. */\n>> > > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n>> > > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n>> > > > + minIfWidth = in.width - IF_CROP_MAX_W;\n>> > > > + minIfHeight = in.height - IF_CROP_MAX_H;\n>> > > > + while (ifHeight >= minIfHeight) {\n>> > > > + /*\n>> > > > + * \\todo This procedure is probably broken:\n>> > > > + *\n>> https://github.com/intel/intel-ipu3-pipecfg/issues/2\n>> > > > + */\n>> > > > + while (ifWidth >= minIfWidth) {\n>> > > > + Size iif{ ifWidth, ifHeight };\n>> > > > + calculateBDS(pipe, iif, gdc, sf);\n>> > > > + ifWidth -= IF_ALIGN_W;\n>> > > > + }\n>> > > > +\n>> > > > + ifHeight -= IF_ALIGN_H;\n>> > > > + }\n>> > > > +\n>> > > > if (pipeConfigs.size() == 0) {\n>> > > > LOG(IPU3, Error) << \"Failed to calculate pipe\n>> > > configuration\";\n>> > > > return {};\n>> > > > --\n>> > > > 2.31.1\n>> > > >\n>> > > > _______________________________________________\n>> > > > libcamera-devel mailing list\n>> > > > libcamera-devel@lists.libcamera.org\n>> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n>> > >\n>> > > --\n>> > > Regards,\n>> > > Niklas Söderlund\n>> > > _______________________________________________\n>> > > libcamera-devel mailing list\n>> > > libcamera-devel@lists.libcamera.org\n>> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 BA9C5BF829\n\tfor ;\n\tWed, 12 May 2021 04:23:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A1DE6891F;\n\tWed, 12 May 2021 06:23:26 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A46F2602B6\n\tfor ;\n\tWed, 12 May 2021 06:23:24 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id b25so32894108eju.5\n\tfor ;\n\tTue, 11 May 2021 21:23:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"S+AuRFQj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=g1KlT4OFzVxHUbBdr/7x/gJZMKaNLIXT7EFuL9tefzE=;\n\tb=S+AuRFQjVKpFhKgMtkqtISx9OZhuDg2+bFcWQQkbXpgohyeqULLLKAH7/ybr6Ixmxo\n\tvUzrn1yzTfLaYJXOKoK0wfiFu8zYy8HBg1ErKgO3TuNiyp49T6uSgXcVr47b+8+yJkJC\n\t/le2sJ2yTBHLoKd3EmawUgzDisq0QRBZXa/Ho=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=g1KlT4OFzVxHUbBdr/7x/gJZMKaNLIXT7EFuL9tefzE=;\n\tb=W3hSbOk4ImMsf01TE5UPlqWy5itELLe/EdAP7n/xvQMHc9r6Qf/SF6kH1muzQtRH89\n\tu/ZID3lyAwlpxmrymbivWQ5NkYfcZTcjdAegppQEcWTHhg+79ZlD4YXEZzivECbu1IzX\n\tdiPU7c4B13OBBBf1bzMltEAvWkQ0vMFY1iC6+QA6Nz8AIsksyFKQ3QfGJxNQmg/bNkCQ\n\tj9pK4LhK6TszGx+L8l6ozirfv9noStRfqgFmhVmd5UB/4JZMFurb1zp/vNgY3zUwXC7R\n\tuFjoeuk2bc8riXT+PmrYL6Qwd8RuTvGKfc5CIRv41jpZzfx3oByHFcPHj92DheHYa/jR\n\tKHWQ==","X-Gm-Message-State":"AOAM530q7aWAF/KxSRPSmaFHkI1KCh3YuDFgPhclh8pCyd4CjK9n5buk\n\tIUJL6MjGER+eQLV/amRr5HKdtVyasnG+Ck1sfA2OQ8kKGmM=","X-Google-Smtp-Source":"ABdhPJxgJLqnWwVESBhDNPOn1jDrye6G5YLhqwaoQ/Kjl++3UuJo7jr/lpp6LSz9eMfDOv90ydpDWNK5pL4B5Sc8HBg=","X-Received":"by 2002:a17:906:2559:: with SMTP id\n\tj25mr34432950ejb.42.1620793404267; \n\tTue, 11 May 2021 21:23:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 12 May 2021 13:23:14 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============1474617186195534121==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16911,"web_url":"https://patchwork.libcamera.org/comment/16911/","msgid":"<20210512071617.2qpf2nedkq6ascej@uno.localdomain>","date":"2021-05-12T07:16:17","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n Yes sorry\n\nOn Wed, May 12, 2021 at 01:23:14PM +0900, Hirokazu Honda wrote:\n> >> > > > + unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> >> > > > + unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> >> > > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> >> > > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> >> > >\n> >> >\n> >> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?\n> >>\n> >> I would be careful and not use 0, but probably 2 and 4 as it seems to be\n> >> the\n> >> vertical and horizontal alignments the ImgU requires\n> >>\n> >> > If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes\n> >> > very large.\n> >> > Ditto to height.\n> >>\n> >> Laurent had the same question, and we received patch in the past that\n> >> was probably related to an issue like this one, if the input size is\n> >> smaller than 540 (as IF_CROP_MAX_H=540).\n> >> https://patchwork.libcamera.org/patch/11620/\n> >>\n> >> I've opened (yet) a new issue on the python script this code is based\n> >> on to have Intel's opinion on which is the min IF crop size\n> >> https://github.com/intel/intel-ipu3-pipecfg/issues/3\n> >>\n> >>\n> > The variable in the python script is (signed) integer.\n> > So overflow doesn't happen.\n> > The code may work even if width < IF_CROP_MAX_W (I don't check the code\n> > any more than it)?\n> > On the other hand, our code doesn't work because the following while-loop\n> > doesn't loop..?\n> > I see your point. It is the best to set the minimum allowed value after we\n> > get their feedback.\n> > But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround to the\n> > overflow with todo comment?\n> > Well, since the code of causing the overflow is in top-of-tree, I don't\n> > mind merging this.\n> >\n> >\n>\n> Ping. Can you respond to this before you merge this patch?\n>\n\nI've taken your suggestion in\n\nI now have got in my wip v3 version:\n\n+ unsigned int minIfWidth = std::min(IF_ALIGN_W,\n+ in.width - IF_CROP_MAX_W);\n+ unsigned int minIfHeight = std::min(IF_ALIGN_H,\n+ in.height - IF_CROP_MAX_H);\n\nI will send it out anyway before merging.\n\n>\n> > -Hiro\n> >\n> > I'm thinking of fixing this issue, and the other ones reported during\n> >> review of this series (and reflected as issues on the script's github)\n> >> on top.\n> >>\n> >> I'm overly-concerned with the idea of deviating from the script. I've\n> >> received several comments on how things could have been done better,\n> >> but I'm pushing back as I want this code to be as similar as possible\n> >> to the intel's script, otherwise every 4 months when (I have to)\n> >> compare the two implementation my brain will melt even more than\n> >> usual.\n> >>\n> >> Result: the code is of awful quality as the script is.\n> >>\n> >> I re-state the requirement from Intel to provide documentation on\n> >> how the ImgU sizes have to be computed instead of a buggy script\n> >> (which I apreciate a lot don't get me wrong, I'm sure the author has\n> >> put some of his free time to implement that as I'm quite sure nobody\n> >> in there apreciate spending more company time on IPU3, but the result\n> >> is a best effort implementation nobody is proud of).\n> >>\n> >> Even better if they could do directly in libcamera... a man can\n> >> dream... So far we received close to 0 attention from them and surely\n> >> we have no leverage from here to convince them of the contrary :)\n> >> Maybe you could push a little to have them consider us a little\n> >> bit more ?\n> >>\n> >> Thanks\n> >> j\n> >>\n> >> >\n> >> >\n> >> > > > while (ifWidth >= minIfWidth) {\n> >> > > > - Size iif{ ifWidth, ifHeight };\n> >> > > > - calculateBDS(pipe, iif, gdc, sf);\n> >> > > > + while (ifHeight >= minIfHeight) {\n> >> > > > + Size iif{ ifWidth, ifHeight };\n> >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> >> > > > + ifHeight -= IF_ALIGN_H;\n> >> > > > + }\n> >> > > >\n> >> > > > ifWidth -= IF_ALIGN_W;\n> >> > > > }\n> >> > > >\n> >> > > > + /* Repeat search by scaling width first. */\n> >> > > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> >> > > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> >> > > > + minIfWidth = in.width - IF_CROP_MAX_W;\n> >> > > > + minIfHeight = in.height - IF_CROP_MAX_H;\n> >> > > > + while (ifHeight >= minIfHeight) {\n> >> > > > + /*\n> >> > > > + * \\todo This procedure is probably broken:\n> >> > > > + *\n> >> https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> >> > > > + */\n> >> > > > + while (ifWidth >= minIfWidth) {\n> >> > > > + Size iif{ ifWidth, ifHeight };\n> >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> >> > > > + ifWidth -= IF_ALIGN_W;\n> >> > > > + }\n> >> > > > +\n> >> > > > + ifHeight -= IF_ALIGN_H;\n> >> > > > + }\n> >> > > > +\n> >> > > > if (pipeConfigs.size() == 0) {\n> >> > > > LOG(IPU3, Error) << \"Failed to calculate pipe\n> >> > > configuration\";\n> >> > > > return {};\n> >> > > > --\n> >> > > > 2.31.1\n> >> > > >\n> >> > > > _______________________________________________\n> >> > > > libcamera-devel mailing list\n> >> > > > libcamera-devel@lists.libcamera.org\n> >> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >> > >\n> >> > > --\n> >> > > Regards,\n> >> > > Niklas Söderlund\n> >> > > _______________________________________________\n> >> > > libcamera-devel mailing list\n> >> > > libcamera-devel@lists.libcamera.org\n> >> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 2E726C31E4\n\tfor ;\n\tWed, 12 May 2021 07:15:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CE5D6891E;\n\tWed, 12 May 2021 09:15:36 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0451D6153C\n\tfor ;\n\tWed, 12 May 2021 09:15:34 +0200 (CEST)","from uno.localdomain (host-79-50-130-20.retail.telecomitalia.it\n\t[79.50.130.20]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id CB460200008;\n\tWed, 12 May 2021 07:15:33 +0000 (UTC)"],"Date":"Wed, 12 May 2021 09:16:17 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210512071617.2qpf2nedkq6ascej@uno.localdomain>","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16912,"web_url":"https://patchwork.libcamera.org/comment/16912/","msgid":"<20210512074625.f5arprzok6tkkq7c@uno.localdomain>","date":"2021-05-12T07:46:25","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro, everyone\n\nOn Wed, May 12, 2021 at 09:16:17AM +0200, Jacopo Mondi wrote:\n> Hi Hiro,\n> Yes sorry\n>\n> On Wed, May 12, 2021 at 01:23:14PM +0900, Hirokazu Honda wrote:\n> > >> > > > + unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > >> > > > + unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > >> > > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > >> > > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> > >> > >\n> > >> >\n> > >> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?\n> > >>\n> > >> I would be careful and not use 0, but probably 2 and 4 as it seems to be\n> > >> the\n> > >> vertical and horizontal alignments the ImgU requires\n> > >>\n> > >> > If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes\n> > >> > very large.\n> > >> > Ditto to height.\n> > >>\n> > >> Laurent had the same question, and we received patch in the past that\n> > >> was probably related to an issue like this one, if the input size is\n> > >> smaller than 540 (as IF_CROP_MAX_H=540).\n> > >> https://patchwork.libcamera.org/patch/11620/\n> > >>\n> > >> I've opened (yet) a new issue on the python script this code is based\n> > >> on to have Intel's opinion on which is the min IF crop size\n> > >> https://github.com/intel/intel-ipu3-pipecfg/issues/3\n> > >>\n> > >>\n> > > The variable in the python script is (signed) integer.\n> > > So overflow doesn't happen.\n> > > The code may work even if width < IF_CROP_MAX_W (I don't check the code\n> > > any more than it)?\n> > > On the other hand, our code doesn't work because the following while-loop\n> > > doesn't loop..?\n> > > I see your point. It is the best to set the minimum allowed value after we\n> > > get their feedback.\n> > > But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround to the\n> > > overflow with todo comment?\n> > > Well, since the code of causing the overflow is in top-of-tree, I don't\n> > > mind merging this.\n> > >\n> > >\n> >\n> > Ping. Can you respond to this before you merge this patch?\n> >\n>\n> I've taken your suggestion in\n>\n> I now have got in my wip v3 version:\n>\n> + unsigned int minIfWidth = std::min(IF_ALIGN_W,\n> + in.width - IF_CROP_MAX_W);\n> + unsigned int minIfHeight = std::min(IF_ALIGN_H,\n> + in.height - IF_CROP_MAX_H);\n>\n> I will send it out anyway before merging.\n>\n\nRun some more testing. With the above change, the number of\nresolutions that are actually tested quickly becomes not manageable at\nrun time.\n\nJust starting the HAL takes 5 seconds per each camera and the number\nof tested configurations goes up to HALF A MILLION (for each\nresolutions combination we test at HAL startup time).\n\nThe overflow helps skipping a lot of configurations, with a maximum of\n30.000 or so. Which is anyway awful but acceptable as the HAL startup\ndelay is barely noticeable.\n\nI'm not sure how to move forward, if we just skip heights <\nIF_CROP_MAX_H (which is 540) to avoid the overflow, we would rule out\nsensors that produces images smaller than 540. This happens today\nalready so it won't technically be a regression, it's just bad.\n\n> >\n> > > -Hiro\n> > >\n> > > I'm thinking of fixing this issue, and the other ones reported during\n> > >> review of this series (and reflected as issues on the script's github)\n> > >> on top.\n> > >>\n> > >> I'm overly-concerned with the idea of deviating from the script. I've\n> > >> received several comments on how things could have been done better,\n> > >> but I'm pushing back as I want this code to be as similar as possible\n> > >> to the intel's script, otherwise every 4 months when (I have to)\n> > >> compare the two implementation my brain will melt even more than\n> > >> usual.\n> > >>\n> > >> Result: the code is of awful quality as the script is.\n> > >>\n> > >> I re-state the requirement from Intel to provide documentation on\n> > >> how the ImgU sizes have to be computed instead of a buggy script\n> > >> (which I apreciate a lot don't get me wrong, I'm sure the author has\n> > >> put some of his free time to implement that as I'm quite sure nobody\n> > >> in there apreciate spending more company time on IPU3, but the result\n> > >> is a best effort implementation nobody is proud of).\n> > >>\n> > >> Even better if they could do directly in libcamera... a man can\n> > >> dream... So far we received close to 0 attention from them and surely\n> > >> we have no leverage from here to convince them of the contrary :)\n> > >> Maybe you could push a little to have them consider us a little\n> > >> bit more ?\n> > >>\n> > >> Thanks\n> > >> j\n> > >>\n> > >> >\n> > >> >\n> > >> > > > while (ifWidth >= minIfWidth) {\n> > >> > > > - Size iif{ ifWidth, ifHeight };\n> > >> > > > - calculateBDS(pipe, iif, gdc, sf);\n> > >> > > > + while (ifHeight >= minIfHeight) {\n> > >> > > > + Size iif{ ifWidth, ifHeight };\n> > >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > >> > > > + ifHeight -= IF_ALIGN_H;\n> > >> > > > + }\n> > >> > > >\n> > >> > > > ifWidth -= IF_ALIGN_W;\n> > >> > > > }\n> > >> > > >\n> > >> > > > + /* Repeat search by scaling width first. */\n> > >> > > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > >> > > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > >> > > > + minIfWidth = in.width - IF_CROP_MAX_W;\n> > >> > > > + minIfHeight = in.height - IF_CROP_MAX_H;\n> > >> > > > + while (ifHeight >= minIfHeight) {\n> > >> > > > + /*\n> > >> > > > + * \\todo This procedure is probably broken:\n> > >> > > > + *\n> > >> https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> > >> > > > + */\n> > >> > > > + while (ifWidth >= minIfWidth) {\n> > >> > > > + Size iif{ ifWidth, ifHeight };\n> > >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > >> > > > + ifWidth -= IF_ALIGN_W;\n> > >> > > > + }\n> > >> > > > +\n> > >> > > > + ifHeight -= IF_ALIGN_H;\n> > >> > > > + }\n> > >> > > > +\n> > >> > > > if (pipeConfigs.size() == 0) {\n> > >> > > > LOG(IPU3, Error) << \"Failed to calculate pipe\n> > >> > > configuration\";\n> > >> > > > return {};\n> > >> > > > --\n> > >> > > > 2.31.1\n> > >> > > >\n> > >> > > > _______________________________________________\n> > >> > > > libcamera-devel mailing list\n> > >> > > > libcamera-devel@lists.libcamera.org\n> > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >> > >\n> > >> > > --\n> > >> > > Regards,\n> > >> > > Niklas Söderlund\n> > >> > > _______________________________________________\n> > >> > > libcamera-devel mailing list\n> > >> > > libcamera-devel@lists.libcamera.org\n> > >> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >> > >\n> > >>\n> > >\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 65B03C31E3\n\tfor ;\n\tWed, 12 May 2021 07:45:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A21156891A;\n\tWed, 12 May 2021 09:45:43 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A679261538\n\tfor ;\n\tWed, 12 May 2021 09:45:42 +0200 (CEST)","from uno.localdomain (host-79-50-130-20.retail.telecomitalia.it\n\t[79.50.130.20]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id C92DD1C0006;\n\tWed, 12 May 2021 07:45:41 +0000 (UTC)"],"X-Originating-IP":"79.50.130.20","Date":"Wed, 12 May 2021 09:46:25 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210512074625.f5arprzok6tkkq7c@uno.localdomain>","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>\n\t\n\t\n\t<20210512071617.2qpf2nedkq6ascej@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210512071617.2qpf2nedkq6ascej@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16913,"web_url":"https://patchwork.libcamera.org/comment/16913/","msgid":"","date":"2021-05-12T07:56:19","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Wed, May 12, 2021 at 4:45 PM Jacopo Mondi wrote:\n\n> Hi Hiro, everyone\n>\n> On Wed, May 12, 2021 at 09:16:17AM +0200, Jacopo Mondi wrote:\n> > Hi Hiro,\n> > Yes sorry\n> >\n> > On Wed, May 12, 2021 at 01:23:14PM +0900, Hirokazu Honda wrote:\n> > > >> > > > + unsigned int ifWidth = utils::alignUp(in.width,\n> IF_ALIGN_W);\n> > > >> > > > + unsigned int ifHeight = utils::alignUp(in.height,\n> IF_ALIGN_H);\n> > > >> > > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > >> > > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> > > >> > >\n> > > >> >\n> > > >> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?\n> > > >>\n> > > >> I would be careful and not use 0, but probably 2 and 4 as it seems\n> to be\n> > > >> the\n> > > >> vertical and horizontal alignments the ImgU requires\n> > > >>\n> > > >> > If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth\n> becomes\n> > > >> > very large.\n> > > >> > Ditto to height.\n> > > >>\n> > > >> Laurent had the same question, and we received patch in the past\n> that\n> > > >> was probably related to an issue like this one, if the input size is\n> > > >> smaller than 540 (as IF_CROP_MAX_H=540).\n> > > >> https://patchwork.libcamera.org/patch/11620/\n> > > >>\n> > > >> I've opened (yet) a new issue on the python script this code is\n> based\n> > > >> on to have Intel's opinion on which is the min IF crop size\n> > > >> https://github.com/intel/intel-ipu3-pipecfg/issues/3\n> > > >>\n> > > >>\n> > > > The variable in the python script is (signed) integer.\n> > > > So overflow doesn't happen.\n> > > > The code may work even if width < IF_CROP_MAX_W (I don't check the\n> code\n> > > > any more than it)?\n> > > > On the other hand, our code doesn't work because the following\n> while-loop\n> > > > doesn't loop..?\n> > > > I see your point. It is the best to set the minimum allowed value\n> after we\n> > > > get their feedback.\n> > > > But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround\n> to the\n> > > > overflow with todo comment?\n> > > > Well, since the code of causing the overflow is in top-of-tree, I\n> don't\n> > > > mind merging this.\n> > > >\n> > > >\n> > >\n> > > Ping. Can you respond to this before you merge this patch?\n> > >\n> >\n> > I've taken your suggestion in\n> >\n> > I now have got in my wip v3 version:\n> >\n> > + unsigned int minIfWidth = std::min(IF_ALIGN_W,\n> > + in.width - IF_CROP_MAX_W);\n> > + unsigned int minIfHeight = std::min(IF_ALIGN_H,\n> > + in.height - IF_CROP_MAX_H);\n> >\n> > I will send it out anyway before merging.\n> >\n>\n> Run some more testing. With the above change, the number of\n> resolutions that are actually tested quickly becomes not manageable at\n> run time.\n>\n> Just starting the HAL takes 5 seconds per each camera and the number\n> of tested configurations goes up to HALF A MILLION (for each\n> resolutions combination we test at HAL startup time).\n>\n> The overflow helps skipping a lot of configurations, with a maximum of\n> 30.000 or so. Which is anyway awful but acceptable as the HAL startup\n> delay is barely noticeable.\n>\n> I'm not sure how to move forward, if we just skip heights <\n> IF_CROP_MAX_H (which is 540) to avoid the overflow, we would rule out\n> sensors that produces images smaller than 540. This happens today\n> already so it won't technically be a regression, it's just bad.\n>\n>\nWow... it is pretty bad.\nIMHO, we can go ahead temporarily with the overflow leaving a todo comment\nand let's file a bug about it as this is not a regression introduced by\nthis.\nWe may want to discuss in the issue tracker.\n\n-Hiro\n\n> >\n> > > > -Hiro\n> > > >\n> > > > I'm thinking of fixing this issue, and the other ones reported during\n> > > >> review of this series (and reflected as issues on the script's\n> github)\n> > > >> on top.\n> > > >>\n> > > >> I'm overly-concerned with the idea of deviating from the script.\n> I've\n> > > >> received several comments on how things could have been done better,\n> > > >> but I'm pushing back as I want this code to be as similar as\n> possible\n> > > >> to the intel's script, otherwise every 4 months when (I have to)\n> > > >> compare the two implementation my brain will melt even more than\n> > > >> usual.\n> > > >>\n> > > >> Result: the code is of awful quality as the script is.\n> > > >>\n> > > >> I re-state the requirement from Intel to provide documentation on\n> > > >> how the ImgU sizes have to be computed instead of a buggy script\n> > > >> (which I apreciate a lot don't get me wrong, I'm sure the author has\n> > > >> put some of his free time to implement that as I'm quite sure nobody\n> > > >> in there apreciate spending more company time on IPU3, but the\n> result\n> > > >> is a best effort implementation nobody is proud of).\n> > > >>\n> > > >> Even better if they could do directly in libcamera... a man can\n> > > >> dream... So far we received close to 0 attention from them and\n> surely\n> > > >> we have no leverage from here to convince them of the contrary :)\n> > > >> Maybe you could push a little to have them consider us a little\n> > > >> bit more ?\n> > > >>\n> > > >> Thanks\n> > > >> j\n> > > >>\n> > > >> >\n> > > >> >\n> > > >> > > > while (ifWidth >= minIfWidth) {\n> > > >> > > > - Size iif{ ifWidth, ifHeight };\n> > > >> > > > - calculateBDS(pipe, iif, gdc, sf);\n> > > >> > > > + while (ifHeight >= minIfHeight) {\n> > > >> > > > + Size iif{ ifWidth, ifHeight };\n> > > >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > > >> > > > + ifHeight -= IF_ALIGN_H;\n> > > >> > > > + }\n> > > >> > > >\n> > > >> > > > ifWidth -= IF_ALIGN_W;\n> > > >> > > > }\n> > > >> > > >\n> > > >> > > > + /* Repeat search by scaling width first. */\n> > > >> > > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > >> > > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > >> > > > + minIfWidth = in.width - IF_CROP_MAX_W;\n> > > >> > > > + minIfHeight = in.height - IF_CROP_MAX_H;\n> > > >> > > > + while (ifHeight >= minIfHeight) {\n> > > >> > > > + /*\n> > > >> > > > + * \\todo This procedure is probably broken:\n> > > >> > > > + *\n> > > >> https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> > > >> > > > + */\n> > > >> > > > + while (ifWidth >= minIfWidth) {\n> > > >> > > > + Size iif{ ifWidth, ifHeight };\n> > > >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > > >> > > > + ifWidth -= IF_ALIGN_W;\n> > > >> > > > + }\n> > > >> > > > +\n> > > >> > > > + ifHeight -= IF_ALIGN_H;\n> > > >> > > > + }\n> > > >> > > > +\n> > > >> > > > if (pipeConfigs.size() == 0) {\n> > > >> > > > LOG(IPU3, Error) << \"Failed to calculate pipe\n> > > >> > > configuration\";\n> > > >> > > > return {};\n> > > >> > > > --\n> > > >> > > > 2.31.1\n> > > >> > > >\n> > > >> > > > _______________________________________________\n> > > >> > > > libcamera-devel mailing list\n> > > >> > > > libcamera-devel@lists.libcamera.org\n> > > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >> > >\n> > > >> > > --\n> > > >> > > Regards,\n> > > >> > > Niklas Söderlund\n> > > >> > > _______________________________________________\n> > > >> > > libcamera-devel mailing list\n> > > >> > > libcamera-devel@lists.libcamera.org\n> > > >> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >> > >\n> > > >>\n> > > >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 D6366C31E4\n\tfor ;\n\tWed, 12 May 2021 07:56:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39E386891F;\n\tWed, 12 May 2021 09:56:31 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10C5E61538\n\tfor ;\n\tWed, 12 May 2021 09:56:30 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id s20so28080192ejr.9\n\tfor ;\n\tWed, 12 May 2021 00:56:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"fvHwmQNE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=QoyGYGrz92dC+yE88lsi4oNQMGP7mcjtZnCCRIdLG+c=;\n\tb=fvHwmQNEI5/9KJ7kZ/aVtgGx2pzgNz1ZN+11NicSx0d5D+ePQ1P4weaWbpOVglOyUT\n\tY+C0f3STsyQlS9sjJNhcftB7INZF+tsx77o7M4fL8Wq3+s/uvAjxaZ7eGS93PlWhLXzY\n\tOX1TEnhXJXhU6Fdq2GfyiT1nKDHqy2ion0eAg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=QoyGYGrz92dC+yE88lsi4oNQMGP7mcjtZnCCRIdLG+c=;\n\tb=iT3Gq6J2FXW/spwPRkmngyfhbBbpY43ic1k5oiF3G4BlXuy+9uaDciS6aQVT+OwMWM\n\tr9YYecATgVZhX3/05p/MY4Bw18Qg1r8m+hUuzrnlk4qIco1XFEJT9nioBrIFsW9f8m5k\n\tpKw4nhKDxRj3XOSwcl7fKj09cKoLsP6VYC3GEM0KOvhCsJD/lR2oi8MuJcvGrlcZN4px\n\t7rMXUIq/Iy3DuaxNuwWm871Kg6tMWYRfWz+x2c6v6KN4wZfd8vBF+Abwjsmjfo4cY/9B\n\tHU0kZ0R8+EaYFAc3mFwIOjx9oFi0DdJukpZeQBbWxG/tLNli6qM8EustPkyh5OFiWoIw\n\tLJtw==","X-Gm-Message-State":"AOAM533Z+xYqShipP0t0SY8FuiRq2eQaH765nLpF9v7TDj3B4Qn1TPGv\n\teXSxr3LnWA4QxoDgATDHAaz5T0lSfMYsM+DQKTqjKQ==","X-Google-Smtp-Source":"ABdhPJx69xU1C7cmcPbS0/vBmBCRUUmEwyghiCFfLzkSPlqxEUrusZpfqlNncE3rVoJ/zuD5JbzbM24y8+u/5UyAdJQ=","X-Received":"by 2002:a17:906:538a:: with SMTP id\n\tg10mr11058900ejo.243.1620806189591; \n\tWed, 12 May 2021 00:56:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>\n\t\n\t\n\t<20210512071617.2qpf2nedkq6ascej@uno.localdomain>\n\t<20210512074625.f5arprzok6tkkq7c@uno.localdomain>","In-Reply-To":"<20210512074625.f5arprzok6tkkq7c@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 12 May 2021 16:56:19 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============2930506444659826062==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16914,"web_url":"https://patchwork.libcamera.org/comment/16914/","msgid":"<20210512084606.hohgjkct4dzp67wx@uno.localdomain>","date":"2021-05-12T08:46:06","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, May 12, 2021 at 04:56:19PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo,\n>\n> On Wed, May 12, 2021 at 4:45 PM Jacopo Mondi wrote:\n>\n> > Hi Hiro, everyone\n> >\n> > On Wed, May 12, 2021 at 09:16:17AM +0200, Jacopo Mondi wrote:\n> > > Hi Hiro,\n> > > Yes sorry\n> > >\n> > > On Wed, May 12, 2021 at 01:23:14PM +0900, Hirokazu Honda wrote:\n> > > > >> > > > + unsigned int ifWidth = utils::alignUp(in.width,\n> > IF_ALIGN_W);\n> > > > >> > > > + unsigned int ifHeight = utils::alignUp(in.height,\n> > IF_ALIGN_H);\n> > > > >> > > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > > >> > > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> > > > >> > >\n> > > > >> >\n> > > > >> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?\n> > > > >>\n> > > > >> I would be careful and not use 0, but probably 2 and 4 as it seems\n> > to be\n> > > > >> the\n> > > > >> vertical and horizontal alignments the ImgU requires\n> > > > >>\n> > > > >> > If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth\n> > becomes\n> > > > >> > very large.\n> > > > >> > Ditto to height.\n> > > > >>\n> > > > >> Laurent had the same question, and we received patch in the past\n> > that\n> > > > >> was probably related to an issue like this one, if the input size is\n> > > > >> smaller than 540 (as IF_CROP_MAX_H=540).\n> > > > >> https://patchwork.libcamera.org/patch/11620/\n> > > > >>\n> > > > >> I've opened (yet) a new issue on the python script this code is\n> > based\n> > > > >> on to have Intel's opinion on which is the min IF crop size\n> > > > >> https://github.com/intel/intel-ipu3-pipecfg/issues/3\n> > > > >>\n> > > > >>\n> > > > > The variable in the python script is (signed) integer.\n> > > > > So overflow doesn't happen.\n> > > > > The code may work even if width < IF_CROP_MAX_W (I don't check the\n> > code\n> > > > > any more than it)?\n> > > > > On the other hand, our code doesn't work because the following\n> > while-loop\n> > > > > doesn't loop..?\n> > > > > I see your point. It is the best to set the minimum allowed value\n> > after we\n> > > > > get their feedback.\n> > > > > But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround\n> > to the\n> > > > > overflow with todo comment?\n> > > > > Well, since the code of causing the overflow is in top-of-tree, I\n> > don't\n> > > > > mind merging this.\n> > > > >\n> > > > >\n> > > >\n> > > > Ping. Can you respond to this before you merge this patch?\n> > > >\n> > >\n> > > I've taken your suggestion in\n> > >\n> > > I now have got in my wip v3 version:\n> > >\n> > > + unsigned int minIfWidth = std::min(IF_ALIGN_W,\n> > > + in.width - IF_CROP_MAX_W);\n> > > + unsigned int minIfHeight = std::min(IF_ALIGN_H,\n> > > + in.height - IF_CROP_MAX_H);\n> > >\n> > > I will send it out anyway before merging.\n> > >\n> >\n> > Run some more testing. With the above change, the number of\n> > resolutions that are actually tested quickly becomes not manageable at\n> > run time.\n> >\n> > Just starting the HAL takes 5 seconds per each camera and the number\n> > of tested configurations goes up to HALF A MILLION (for each\n> > resolutions combination we test at HAL startup time).\n> >\n> > The overflow helps skipping a lot of configurations, with a maximum of\n> > 30.000 or so. Which is anyway awful but acceptable as the HAL startup\n> > delay is barely noticeable.\n> >\n> > I'm not sure how to move forward, if we just skip heights <\n> > IF_CROP_MAX_H (which is 540) to avoid the overflow, we would rule out\n> > sensors that produces images smaller than 540. This happens today\n> > already so it won't technically be a regression, it's just bad.\n> >\n> >\n> Wow... it is pretty bad.\n\nyes indeed.\n\n> IMHO, we can go ahead temporarily with the overflow leaving a todo comment\n> and let's file a bug about it as this is not a regression introduced by\n> this.\n> We may want to discuss in the issue tracker.\n\nI've not recorded it in a bug, and will refer to that in the code\nhttps://bugs.libcamera.org/show_bug.cgi?id=32\n\nFor the moment I feel it would be safer to skip all input resolutions <\nIF_CROP_MAX\n\nThanks\n j\n\n>\n> -Hiro\n>\n> > >\n> > > > > -Hiro\n> > > > >\n> > > > > I'm thinking of fixing this issue, and the other ones reported during\n> > > > >> review of this series (and reflected as issues on the script's\n> > github)\n> > > > >> on top.\n> > > > >>\n> > > > >> I'm overly-concerned with the idea of deviating from the script.\n> > I've\n> > > > >> received several comments on how things could have been done better,\n> > > > >> but I'm pushing back as I want this code to be as similar as\n> > possible\n> > > > >> to the intel's script, otherwise every 4 months when (I have to)\n> > > > >> compare the two implementation my brain will melt even more than\n> > > > >> usual.\n> > > > >>\n> > > > >> Result: the code is of awful quality as the script is.\n> > > > >>\n> > > > >> I re-state the requirement from Intel to provide documentation on\n> > > > >> how the ImgU sizes have to be computed instead of a buggy script\n> > > > >> (which I apreciate a lot don't get me wrong, I'm sure the author has\n> > > > >> put some of his free time to implement that as I'm quite sure nobody\n> > > > >> in there apreciate spending more company time on IPU3, but the\n> > result\n> > > > >> is a best effort implementation nobody is proud of).\n> > > > >>\n> > > > >> Even better if they could do directly in libcamera... a man can\n> > > > >> dream... So far we received close to 0 attention from them and\n> > surely\n> > > > >> we have no leverage from here to convince them of the contrary :)\n> > > > >> Maybe you could push a little to have them consider us a little\n> > > > >> bit more ?\n> > > > >>\n> > > > >> Thanks\n> > > > >> j\n> > > > >>\n> > > > >> >\n> > > > >> >\n> > > > >> > > > while (ifWidth >= minIfWidth) {\n> > > > >> > > > - Size iif{ ifWidth, ifHeight };\n> > > > >> > > > - calculateBDS(pipe, iif, gdc, sf);\n> > > > >> > > > + while (ifHeight >= minIfHeight) {\n> > > > >> > > > + Size iif{ ifWidth, ifHeight };\n> > > > >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > > > >> > > > + ifHeight -= IF_ALIGN_H;\n> > > > >> > > > + }\n> > > > >> > > >\n> > > > >> > > > ifWidth -= IF_ALIGN_W;\n> > > > >> > > > }\n> > > > >> > > >\n> > > > >> > > > + /* Repeat search by scaling width first. */\n> > > > >> > > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > > >> > > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > > >> > > > + minIfWidth = in.width - IF_CROP_MAX_W;\n> > > > >> > > > + minIfHeight = in.height - IF_CROP_MAX_H;\n> > > > >> > > > + while (ifHeight >= minIfHeight) {\n> > > > >> > > > + /*\n> > > > >> > > > + * \\todo This procedure is probably broken:\n> > > > >> > > > + *\n> > > > >> https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> > > > >> > > > + */\n> > > > >> > > > + while (ifWidth >= minIfWidth) {\n> > > > >> > > > + Size iif{ ifWidth, ifHeight };\n> > > > >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > > > >> > > > + ifWidth -= IF_ALIGN_W;\n> > > > >> > > > + }\n> > > > >> > > > +\n> > > > >> > > > + ifHeight -= IF_ALIGN_H;\n> > > > >> > > > + }\n> > > > >> > > > +\n> > > > >> > > > if (pipeConfigs.size() == 0) {\n> > > > >> > > > LOG(IPU3, Error) << \"Failed to calculate pipe\n> > > > >> > > configuration\";\n> > > > >> > > > return {};\n> > > > >> > > > --\n> > > > >> > > > 2.31.1\n> > > > >> > > >\n> > > > >> > > > _______________________________________________\n> > > > >> > > > libcamera-devel mailing list\n> > > > >> > > > libcamera-devel@lists.libcamera.org\n> > > > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > >> > >\n> > > > >> > > --\n> > > > >> > > Regards,\n> > > > >> > > Niklas Söderlund\n> > > > >> > > _______________________________________________\n> > > > >> > > libcamera-devel mailing list\n> > > > >> > > libcamera-devel@lists.libcamera.org\n> > > > >> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > >> > >\n> > > > >>\n> > > > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 2D115C31E3\n\tfor ;\n\tWed, 12 May 2021 08:45:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C4596891A;\n\tWed, 12 May 2021 10:45:24 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98380688E4\n\tfor ;\n\tWed, 12 May 2021 10:45:23 +0200 (CEST)","from uno.localdomain (host-79-50-130-20.retail.telecomitalia.it\n\t[79.50.130.20]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id D14164000E;\n\tWed, 12 May 2021 08:45:22 +0000 (UTC)"],"X-Originating-IP":"79.50.130.20","Date":"Wed, 12 May 2021 10:46:06 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210512084606.hohgjkct4dzp67wx@uno.localdomain>","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>\n\t\n\t\n\t<20210512071617.2qpf2nedkq6ascej@uno.localdomain>\n\t<20210512074625.f5arprzok6tkkq7c@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16922,"web_url":"https://patchwork.libcamera.org/comment/16922/","msgid":"","date":"2021-05-13T02:19:19","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation process","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Wed, May 12, 2021 at 5:45 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Wed, May 12, 2021 at 04:56:19PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo,\n> >\n> > On Wed, May 12, 2021 at 4:45 PM Jacopo Mondi wrote:\n> >\n> > > Hi Hiro, everyone\n> > >\n> > > On Wed, May 12, 2021 at 09:16:17AM +0200, Jacopo Mondi wrote:\n> > > > Hi Hiro,\n> > > > Yes sorry\n> > > >\n> > > > On Wed, May 12, 2021 at 01:23:14PM +0900, Hirokazu Honda wrote:\n> > > > > >> > > > + unsigned int ifWidth = utils::alignUp(in.width,\n> > > IF_ALIGN_W);\n> > > > > >> > > > + unsigned int ifHeight = utils::alignUp(in.height,\n> > > IF_ALIGN_H);\n> > > > > >> > > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > > > > >> > > > + unsigned int minIfHeight = in.height -\n> IF_CROP_MAX_H;\n> > > > > >> > >\n> > > > > >> >\n> > > > > >> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?\n> > > > > >>\n> > > > > >> I would be careful and not use 0, but probably 2 and 4 as it\n> seems\n> > > to be\n> > > > > >> the\n> > > > > >> vertical and horizontal alignments the ImgU requires\n> > > > > >>\n> > > > > >> > If in.width < IF_CROP_MAX_W, an overflow happens and\n> minIfWidth\n> > > becomes\n> > > > > >> > very large.\n> > > > > >> > Ditto to height.\n> > > > > >>\n> > > > > >> Laurent had the same question, and we received patch in the past\n> > > that\n> > > > > >> was probably related to an issue like this one, if the input\n> size is\n> > > > > >> smaller than 540 (as IF_CROP_MAX_H=540).\n> > > > > >> https://patchwork.libcamera.org/patch/11620/\n> > > > > >>\n> > > > > >> I've opened (yet) a new issue on the python script this code is\n> > > based\n> > > > > >> on to have Intel's opinion on which is the min IF crop size\n> > > > > >> https://github.com/intel/intel-ipu3-pipecfg/issues/3\n> > > > > >>\n> > > > > >>\n> > > > > > The variable in the python script is (signed) integer.\n> > > > > > So overflow doesn't happen.\n> > > > > > The code may work even if width < IF_CROP_MAX_W (I don't check\n> the\n> > > code\n> > > > > > any more than it)?\n> > > > > > On the other hand, our code doesn't work because the following\n> > > while-loop\n> > > > > > doesn't loop..?\n> > > > > > I see your point. It is the best to set the minimum allowed value\n> > > after we\n> > > > > > get their feedback.\n> > > > > > But, hmm, shall we set it to IF_ALIGN_W as the temporary\n> workaround\n> > > to the\n> > > > > > overflow with todo comment?\n> > > > > > Well, since the code of causing the overflow is in top-of-tree, I\n> > > don't\n> > > > > > mind merging this.\n> > > > > >\n> > > > > >\n> > > > >\n> > > > > Ping. Can you respond to this before you merge this patch?\n> > > > >\n> > > >\n> > > > I've taken your suggestion in\n> > > >\n> > > > I now have got in my wip v3 version:\n> > > >\n> > > > + unsigned int minIfWidth = std::min(IF_ALIGN_W,\n> > > > + in.width - IF_CROP_MAX_W);\n> > > > + unsigned int minIfHeight = std::min(IF_ALIGN_H,\n> > > > + in.height -\n> IF_CROP_MAX_H);\n> > > >\n> > > > I will send it out anyway before merging.\n> > > >\n> > >\n> > > Run some more testing. With the above change, the number of\n> > > resolutions that are actually tested quickly becomes not manageable at\n> > > run time.\n> > >\n> > > Just starting the HAL takes 5 seconds per each camera and the number\n> > > of tested configurations goes up to HALF A MILLION (for each\n> > > resolutions combination we test at HAL startup time).\n> > >\n> > > The overflow helps skipping a lot of configurations, with a maximum of\n> > > 30.000 or so. Which is anyway awful but acceptable as the HAL startup\n> > > delay is barely noticeable.\n> > >\n> > > I'm not sure how to move forward, if we just skip heights <\n> > > IF_CROP_MAX_H (which is 540) to avoid the overflow, we would rule out\n> > > sensors that produces images smaller than 540. This happens today\n> > > already so it won't technically be a regression, it's just bad.\n> > >\n> > >\n> > Wow... it is pretty bad.\n>\n> yes indeed.\n>\n> > IMHO, we can go ahead temporarily with the overflow leaving a todo\n> comment\n> > and let's file a bug about it as this is not a regression introduced by\n> > this.\n> > We may want to discuss in the issue tracker.\n>\n> I've not recorded it in a bug, and will refer to that in the code\n> https://bugs.libcamera.org/show_bug.cgi?id=32\n>\n> For the moment I feel it would be safer to skip all input resolutions <\n> IF_CROP_MAX\n>\n>\nAck. Thanks!\n\nReviewed-by: Hirokazu Honda \n\n-Hiro\n\n\n> Thanks\n> j\n>\n> >\n> > -Hiro\n> >\n> > > >\n> > > > > > -Hiro\n> > > > > >\n> > > > > > I'm thinking of fixing this issue, and the other ones reported\n> during\n> > > > > >> review of this series (and reflected as issues on the script's\n> > > github)\n> > > > > >> on top.\n> > > > > >>\n> > > > > >> I'm overly-concerned with the idea of deviating from the script.\n> > > I've\n> > > > > >> received several comments on how things could have been done\n> better,\n> > > > > >> but I'm pushing back as I want this code to be as similar as\n> > > possible\n> > > > > >> to the intel's script, otherwise every 4 months when (I have to)\n> > > > > >> compare the two implementation my brain will melt even more than\n> > > > > >> usual.\n> > > > > >>\n> > > > > >> Result: the code is of awful quality as the script is.\n> > > > > >>\n> > > > > >> I re-state the requirement from Intel to provide documentation\n> on\n> > > > > >> how the ImgU sizes have to be computed instead of a buggy script\n> > > > > >> (which I apreciate a lot don't get me wrong, I'm sure the\n> author has\n> > > > > >> put some of his free time to implement that as I'm quite sure\n> nobody\n> > > > > >> in there apreciate spending more company time on IPU3, but the\n> > > result\n> > > > > >> is a best effort implementation nobody is proud of).\n> > > > > >>\n> > > > > >> Even better if they could do directly in libcamera... a man can\n> > > > > >> dream... So far we received close to 0 attention from them and\n> > > surely\n> > > > > >> we have no leverage from here to convince them of the contrary\n> :)\n> > > > > >> Maybe you could push a little to have them consider us a little\n> > > > > >> bit more ?\n> > > > > >>\n> > > > > >> Thanks\n> > > > > >> j\n> > > > > >>\n> > > > > >> >\n> > > > > >> >\n> > > > > >> > > > while (ifWidth >= minIfWidth) {\n> > > > > >> > > > - Size iif{ ifWidth, ifHeight };\n> > > > > >> > > > - calculateBDS(pipe, iif, gdc, sf);\n> > > > > >> > > > + while (ifHeight >= minIfHeight) {\n> > > > > >> > > > + Size iif{ ifWidth, ifHeight };\n> > > > > >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > > > > >> > > > + ifHeight -= IF_ALIGN_H;\n> > > > > >> > > > + }\n> > > > > >> > > >\n> > > > > >> > > > ifWidth -= IF_ALIGN_W;\n> > > > > >> > > > }\n> > > > > >> > > >\n> > > > > >> > > > + /* Repeat search by scaling width first. */\n> > > > > >> > > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > > > > >> > > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > > > > >> > > > + minIfWidth = in.width - IF_CROP_MAX_W;\n> > > > > >> > > > + minIfHeight = in.height - IF_CROP_MAX_H;\n> > > > > >> > > > + while (ifHeight >= minIfHeight) {\n> > > > > >> > > > + /*\n> > > > > >> > > > + * \\todo This procedure is probably broken:\n> > > > > >> > > > + *\n> > > > > >> https://github.com/intel/intel-ipu3-pipecfg/issues/2\n> > > > > >> > > > + */\n> > > > > >> > > > + while (ifWidth >= minIfWidth) {\n> > > > > >> > > > + Size iif{ ifWidth, ifHeight };\n> > > > > >> > > > + calculateBDS(pipe, iif, gdc, sf);\n> > > > > >> > > > + ifWidth -= IF_ALIGN_W;\n> > > > > >> > > > + }\n> > > > > >> > > > +\n> > > > > >> > > > + ifHeight -= IF_ALIGN_H;\n> > > > > >> > > > + }\n> > > > > >> > > > +\n> > > > > >> > > > if (pipeConfigs.size() == 0) {\n> > > > > >> > > > LOG(IPU3, Error) << \"Failed to calculate\n> pipe\n> > > > > >> > > configuration\";\n> > > > > >> > > > return {};\n> > > > > >> > > > --\n> > > > > >> > > > 2.31.1\n> > > > > >> > > >\n> > > > > >> > > > _______________________________________________\n> > > > > >> > > > libcamera-devel mailing list\n> > > > > >> > > > libcamera-devel@lists.libcamera.org\n> > > > > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > >> > >\n> > > > > >> > > --\n> > > > > >> > > Regards,\n> > > > > >> > > Niklas Söderlund\n> > > > > >> > > _______________________________________________\n> > > > > >> > > libcamera-devel mailing list\n> > > > > >> > > libcamera-devel@lists.libcamera.org\n> > > > > >> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > >> > >\n> > > > > >>\n> > > > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\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 0ECE5C31E7\n\tfor ;\n\tThu, 13 May 2021 02:19:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A77106891D;\n\tThu, 13 May 2021 04:19:31 +0200 (CEST)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A57EA61538\n\tfor ;\n\tThu, 13 May 2021 04:19:29 +0200 (CEST)","by mail-ed1-x52e.google.com with SMTP id s6so29281131edu.10\n\tfor ;\n\tWed, 12 May 2021 19:19:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"AbA+HBNt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=7ZRJnZuRO06F3zfgqJDCqncV3HnIpxN4Tla72rVDq1w=;\n\tb=AbA+HBNtLmVuPQrCkcw64vZwa55wYHnM0/e5UdzpMQsPhYACKimYdbft9tSP/I8c+E\n\t0DxTmeetlzXqhuN8o9zLLAISiHwXFdHYTtU6sX3ROLxdRZ1Qij+C9whbh/u4vDvKl8zA\n\tlyqUh8EDhApt6sW372kFywAJrNznYMFxIo+AE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=7ZRJnZuRO06F3zfgqJDCqncV3HnIpxN4Tla72rVDq1w=;\n\tb=dkIkLild3hs1It04TcCRvEE9u0FXD+d7blObLUaiQbsq0W0ebVgUZXwIHLQ0OrQ89V\n\t5TqEzcYKOg9u9+AT1ljdOG6c6lCiJhASY3UdDBlN5ynySGWwujNSJy1RAimsGJSpb+ld\n\tDzPM1PGs+qc79MAA/jq1cRr3rEw2NWzJGsv1KXVBWEnLgrQUimCaYvCFbu42wt/BX8Xh\n\t4RxWMvDiYihZeEKorVjvSiK+OaPiIeoNEsjUEVBZ2J1YDuIx6QhQu2DCYrRAufijbptX\n\tsL1p7kvOeFChDwkGtJZXIkD/wx6eZz8wM19USDqtDVZ67jjr786ExSWvYrH0u1txfbhG\n\t+5Xw==","X-Gm-Message-State":"AOAM532EBrVKT0kknViCNn9lGrItEmMFnZbGsWHbcpybgrIw4tg3uBIR\n\tPKwGWXn6lxv8SGAo/hpFm0+h4EBuNB/gh51c6NM7RgRX8Jo=","X-Google-Smtp-Source":"ABdhPJzAE2h/9TxYi5wcCQGTGhEbcjzLLTigSaJ9p5xODcSQz33ZZcJNBNAM046LTJCz10d9247vQASAwOG0vFkU+Xg=","X-Received":"by 2002:aa7:c610:: with SMTP id\n\th16mr46577048edq.202.1620872369220; \n\tWed, 12 May 2021 19:19:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506081559.zqrwqbyqum5x4fa3@uno.localdomain>\n\t\n\t\n\t<20210512071617.2qpf2nedkq6ascej@uno.localdomain>\n\t<20210512074625.f5arprzok6tkkq7c@uno.localdomain>\n\t\n\t<20210512084606.hohgjkct4dzp67wx@uno.localdomain>","In-Reply-To":"<20210512084606.hohgjkct4dzp67wx@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 13 May 2021 11:19:19 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update\n\tBDS calculation 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============8447912351625347042==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]