[{"id":19763,"web_url":"https://patchwork.libcamera.org/comment/19763/","msgid":"","date":"2021-09-22T01:50:13","subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Sep 07, 2021 at 09:40:51PM +0200, Jacopo Mondi wrote:\n> As reported by commit 7208e70211a6 (\"libcamera: ipu3: Always use sensor\n> full frame size\") the current implementation of the IPU3 pipeline\n> handler always uses the sensor resolution as the ImgU input frame size in\n> order to work around an issue with the ImgU configuration procedure.\n> \n> Now that the frame selection policy has been modified in the CIO2Device\n> class implementation to comply with the requirements of the ImgU\n> configuration script we can remove the workaround and select the most\n> opportune sensor size to feed the ImgU with.\n\nOn a side note, I missed reviewing the changes in cio2.cpp, doesn't that\nbelong to ipu3.cpp instead ?\n\n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Paul Elder \n> Reviewed-by: Umang Jain \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------\n> 1 file changed, 23 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..291338288685 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\tstatus = Adjusted;\n> \t}\n> \n> -\t/* Validate the requested stream configuration */\n> +\t/*\n> +\t * Validate the requested stream configuration and select the sensor\n> +\t * format by collecting the maximum RAW stream width and height and\n> +\t * picking the closest larger match.\n> +\t *\n> +\t * If no RAW stream is requested use the one of the largest YUV stream,\n> +\t * plus margin pixels for the IF and BDS rectangle to downscale.\n> +\t *\n> +\t * \\todo Clarify the IF and BDS margins requirements.\n> +\t */\n> \tunsigned int rawCount = 0;\n> \tunsigned int yuvCount = 0;\n> \tSize maxYuvSize;\n> @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \n> \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> \t\t\trawCount++;\n> -\t\t\trawSize = cfg.size;\n> +\t\t\trawSize.expandTo(cfg.size);\n\nWe support a single raw stream only, is this change needed ?\n\n> \t\t} else {\n> \t\t\tyuvCount++;\n> \t\t\tmaxYuvSize.expandTo(cfg.size);\n> @@ -269,24 +278,20 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t/*\n> \t * Generate raw configuration from CIO2.\n> \t *\n> -\t * \\todo The image sensor frame size should be selected to optimize\n> -\t * operations based on the sizes of the requested streams. However such\n> -\t * a selection makes the pipeline configuration procedure fail for small\n> -\t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> -\t * operations to stall for some stream size combinations (see the\n> -\t * commit message of the patch that introduced this comment for more\n> -\t * failure examples).\n> +\t * The output YUV streams will be limited in size to the\n> +\t * maximum frame size requested for the RAW stream, if present.\n\nThis could be reflowed.\n\n> \t *\n> -\t * Until the sensor frame size calculation criteria are clarified, when\n> -\t * capturing from ImgU always use the largest possible size which\n> -\t * guarantees better results at the expense of the frame rate and CSI-2\n> -\t * bus bandwidth. When only a raw stream is requested use the requested\n> -\t * size instead, as the ImgU is not involved.\n> +\t * If no raw stream is requested generate a size as large as the maximum\n> +\t * requested YUV size aligned to the ImgU constraints and bound by the\n> +\t * sensor's maximum resolution. See\n> +\t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> \t */\n> -\tif (!yuvCount)\n> -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> -\telse\n> -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration({});\n> +\tif (rawSize.isNull())\n> +\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n\nIs alignedUpTo() the right function ? It will round up the height to a\nmultiple of 540, that doesn't sound right.\n\n> +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n\nSame here.\n\n> +\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n> +\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> \tif (!cio2Configuration_.pixelFormat.isValid())\n> \t\treturn Invalid;\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 208B3BDC71\n\tfor ;\n\tWed, 22 Sep 2021 01:50:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CB186918C;\n\tWed, 22 Sep 2021 03:50:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DD1F6012C\n\tfor ;\n\tWed, 22 Sep 2021 03:50:44 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 10F1FF1;\n\tWed, 22 Sep 2021 03:50:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TynbbXha\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632275444;\n\tbh=0TGSzDO/eF5hUgUdnJYfvoaVHonzVUiUZiLKjKtJKlU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TynbbXhawhVdpkdLfcAN+Fsat+1AY3tJk6A9m1mXoveHHG7J4aHvLS5Yn8i8FAi/d\n\tDkq9zP3EcGLv2EiIRaUxLX/A+tl2ydfHmZDEzx+aV/5wCa/EwVVjuCgoU5axqH5/8e\n\t/u/etIVKT9jkUYt5pyGQen+38UwOSY/qCUXCMXaI=","Date":"Wed, 22 Sep 2021 04:50:13 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210907194107.803730-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","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, umang@pendragon.ideasonboard.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19764,"web_url":"https://patchwork.libcamera.org/comment/19764/","msgid":"","date":"2021-09-22T01:51:26","subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\n(Resending with Umang's correct e-mail address now)\n\nOn Tue, Sep 07, 2021 at 09:40:51PM +0200, Jacopo Mondi wrote:\n> As reported by commit 7208e70211a6 (\"libcamera: ipu3: Always use sensor\n> full frame size\") the current implementation of the IPU3 pipeline\n> handler always uses the sensor resolution as the ImgU input frame size in\n> order to work around an issue with the ImgU configuration procedure.\n> \n> Now that the frame selection policy has been modified in the CIO2Device\n> class implementation to comply with the requirements of the ImgU\n> configuration script we can remove the workaround and select the most\n> opportune sensor size to feed the ImgU with.\n\nOn a side note, I missed reviewing the changes in cio2.cpp, doesn't that\nbelong to ipu3.cpp instead ?\n\n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Paul Elder \n> Reviewed-by: Umang Jain \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------\n> 1 file changed, 23 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..291338288685 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\tstatus = Adjusted;\n> \t}\n> \n> -\t/* Validate the requested stream configuration */\n> +\t/*\n> +\t * Validate the requested stream configuration and select the sensor\n> +\t * format by collecting the maximum RAW stream width and height and\n> +\t * picking the closest larger match.\n> +\t *\n> +\t * If no RAW stream is requested use the one of the largest YUV stream,\n> +\t * plus margin pixels for the IF and BDS rectangle to downscale.\n> +\t *\n> +\t * \\todo Clarify the IF and BDS margins requirements.\n> +\t */\n> \tunsigned int rawCount = 0;\n> \tunsigned int yuvCount = 0;\n> \tSize maxYuvSize;\n> @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \n> \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> \t\t\trawCount++;\n> -\t\t\trawSize = cfg.size;\n> +\t\t\trawSize.expandTo(cfg.size);\n\nWe support a single raw stream only, is this change needed ?\n\n> \t\t} else {\n> \t\t\tyuvCount++;\n> \t\t\tmaxYuvSize.expandTo(cfg.size);\n> @@ -269,24 +278,20 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t/*\n> \t * Generate raw configuration from CIO2.\n> \t *\n> -\t * \\todo The image sensor frame size should be selected to optimize\n> -\t * operations based on the sizes of the requested streams. However such\n> -\t * a selection makes the pipeline configuration procedure fail for small\n> -\t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> -\t * operations to stall for some stream size combinations (see the\n> -\t * commit message of the patch that introduced this comment for more\n> -\t * failure examples).\n> +\t * The output YUV streams will be limited in size to the\n> +\t * maximum frame size requested for the RAW stream, if present.\n\nThis could be reflowed.\n\n> \t *\n> -\t * Until the sensor frame size calculation criteria are clarified, when\n> -\t * capturing from ImgU always use the largest possible size which\n> -\t * guarantees better results at the expense of the frame rate and CSI-2\n> -\t * bus bandwidth. When only a raw stream is requested use the requested\n> -\t * size instead, as the ImgU is not involved.\n> +\t * If no raw stream is requested generate a size as large as the maximum\n> +\t * requested YUV size aligned to the ImgU constraints and bound by the\n> +\t * sensor's maximum resolution. See\n> +\t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> \t */\n> -\tif (!yuvCount)\n> -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> -\telse\n> -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration({});\n> +\tif (rawSize.isNull())\n> +\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n\nIs alignedUpTo() the right function ? It will round up the height to a\nmultiple of 540, that doesn't sound right.\n\n> +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n\nSame here.\n\n> +\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n> +\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> \tif (!cio2Configuration_.pixelFormat.isValid())\n> \t\treturn Invalid;\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 81FDABDC71\n\tfor ;\n\tWed, 22 Sep 2021 01:51:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50D966918C;\n\tWed, 22 Sep 2021 03:51:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AE0D6012C\n\tfor ;\n\tWed, 22 Sep 2021 03:51:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D024AF1;\n\tWed, 22 Sep 2021 03:51:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OVXL7x+Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632275517;\n\tbh=CeSKaL2dTiRY44+1vq47p9qujzlhidLXM/e8BleO9UM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OVXL7x+YNm9ZcNjmED47rg6GZkck0hjrmuK72fQ487Omde7WvQF0shZFpeEJGM9Cc\n\tRpS7Q7W7F1ZxcJEpjfgAQN9uXMD17VC9ZP4ds1eWQHntgQ8HgSiUh7uOdL+kmqp9u3\n\tDr3Kz2zWEpv1Kx4SjduZituS6skrtZu4y1nGzvwc=","Date":"Wed, 22 Sep 2021 04:51:26 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210907194107.803730-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20102,"web_url":"https://patchwork.libcamera.org/comment/20102/","msgid":"<20211011081817.vupfxih3lfb6jl7o@uno.localdomain>","date":"2021-10-11T08:18:17","subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Sorry I forgot to reply to this one\n\nOn Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> (Resending with Umang's correct e-mail address now)\n>\n> On Tue, Sep 07, 2021 at 09:40:51PM +0200, Jacopo Mondi wrote:\n> > As reported by commit 7208e70211a6 (\"libcamera: ipu3: Always use sensor\n> > full frame size\") the current implementation of the IPU3 pipeline\n> > handler always uses the sensor resolution as the ImgU input frame size in\n> > order to work around an issue with the ImgU configuration procedure.\n> >\n> > Now that the frame selection policy has been modified in the CIO2Device\n> > class implementation to comply with the requirements of the ImgU\n> > configuration script we can remove the workaround and select the most\n> > opportune sensor size to feed the ImgU with.\n>\n> On a side note, I missed reviewing the changes in cio2.cpp, doesn't that\n> belong to ipu3.cpp instead ?\n>\n\n$ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat\n src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++\n\n\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Paul Elder \n> > Reviewed-by: Umang Jain \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------\n> > 1 file changed, 23 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index c287bf86e79a..291338288685 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \t\tstatus = Adjusted;\n> > \t}\n> >\n> > -\t/* Validate the requested stream configuration */\n> > +\t/*\n> > +\t * Validate the requested stream configuration and select the sensor\n> > +\t * format by collecting the maximum RAW stream width and height and\n> > +\t * picking the closest larger match.\n> > +\t *\n> > +\t * If no RAW stream is requested use the one of the largest YUV stream,\n> > +\t * plus margin pixels for the IF and BDS rectangle to downscale.\n> > +\t *\n> > +\t * \\todo Clarify the IF and BDS margins requirements.\n> > +\t */\n> > \tunsigned int rawCount = 0;\n> > \tunsigned int yuvCount = 0;\n> > \tSize maxYuvSize;\n> > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >\n> > \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > \t\t\trawCount++;\n> > -\t\t\trawSize = cfg.size;\n> > +\t\t\trawSize.expandTo(cfg.size);\n>\n> We support a single raw stream only, is this change needed ?\n>\n\nIt makes the two branches look the same\n\n> > \t\t} else {\n> > \t\t\tyuvCount++;\n> > \t\t\tmaxYuvSize.expandTo(cfg.size);\n> > @@ -269,24 +278,20 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \t/*\n> > \t * Generate raw configuration from CIO2.\n> > \t *\n> > -\t * \\todo The image sensor frame size should be selected to optimize\n> > -\t * operations based on the sizes of the requested streams. However such\n> > -\t * a selection makes the pipeline configuration procedure fail for small\n> > -\t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> > -\t * operations to stall for some stream size combinations (see the\n> > -\t * commit message of the patch that introduced this comment for more\n> > -\t * failure examples).\n> > +\t * The output YUV streams will be limited in size to the\n> > +\t * maximum frame size requested for the RAW stream, if present.\n>\n> This could be reflowed.\n>\n> > \t *\n> > -\t * Until the sensor frame size calculation criteria are clarified, when\n> > -\t * capturing from ImgU always use the largest possible size which\n> > -\t * guarantees better results at the expense of the frame rate and CSI-2\n> > -\t * bus bandwidth. When only a raw stream is requested use the requested\n> > -\t * size instead, as the ImgU is not involved.\n> > +\t * If no raw stream is requested generate a size as large as the maximum\n> > +\t * requested YUV size aligned to the ImgU constraints and bound by the\n> > +\t * sensor's maximum resolution. See\n> > +\t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> > \t */\n> > -\tif (!yuvCount)\n> > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > -\telse\n> > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration({});\n> > +\tif (rawSize.isNull())\n> > +\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n>\n> Is alignedUpTo() the right function ? It will round up the height to a\n> multiple of 540, that doesn't sound right.\n>\n\nNo it should probably be expandedTo()\n\n> > +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n>\n> Same here.\n>\n\nWhile I guess this one is correct as we need to align up to the\nmargins\n\n> > +\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n> > +\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > \tif (!cio2Configuration_.pixelFormat.isValid())\n> > \t\treturn Invalid;\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 780F1C323E\n\tfor ;\n\tMon, 11 Oct 2021 08:17:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 485A668F4B;\n\tMon, 11 Oct 2021 10:17:30 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3502860502\n\tfor ;\n\tMon, 11 Oct 2021 10:17:29 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 71C6F20008;\n\tMon, 11 Oct 2021 08:17:28 +0000 (UTC)"],"Date":"Mon, 11 Oct 2021 10:18:17 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211011081817.vupfxih3lfb6jl7o@uno.localdomain>","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20114,"web_url":"https://patchwork.libcamera.org/comment/20114/","msgid":"","date":"2021-10-11T14:08:26","subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\n\nOn Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote:\n> On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > (Resending with Umang's correct e-mail address now)\n> >\n> > On Tue, Sep 07, 2021 at 09:40:51PM +0200, Jacopo Mondi wrote:\n> > > As reported by commit 7208e70211a6 (\"libcamera: ipu3: Always use sensor\n> > > full frame size\") the current implementation of the IPU3 pipeline\n> > > handler always uses the sensor resolution as the ImgU input frame size in\n> > > order to work around an issue with the ImgU configuration procedure.\n> > >\n> > > Now that the frame selection policy has been modified in the CIO2Device\n> > > class implementation to comply with the requirements of the ImgU\n> > > configuration script we can remove the workaround and select the most\n> > > opportune sensor size to feed the ImgU with.\n> >\n> > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that\n> > belong to ipu3.cpp instead ?\n> \n> $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat\n> src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++\n\nI meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ?\n\n> > > Signed-off-by: Jacopo Mondi \n> > > Reviewed-by: Paul Elder \n> > > Reviewed-by: Umang Jain \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------\n> > > 1 file changed, 23 insertions(+), 18 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index c287bf86e79a..291338288685 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > \t\tstatus = Adjusted;\n> > > \t}\n> > >\n> > > -\t/* Validate the requested stream configuration */\n> > > +\t/*\n> > > +\t * Validate the requested stream configuration and select the sensor\n> > > +\t * format by collecting the maximum RAW stream width and height and\n> > > +\t * picking the closest larger match.\n> > > +\t *\n> > > +\t * If no RAW stream is requested use the one of the largest YUV stream,\n> > > +\t * plus margin pixels for the IF and BDS rectangle to downscale.\n> > > +\t *\n> > > +\t * \\todo Clarify the IF and BDS margins requirements.\n> > > +\t */\n> > > \tunsigned int rawCount = 0;\n> > > \tunsigned int yuvCount = 0;\n> > > \tSize maxYuvSize;\n> > > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > >\n> > > \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > > \t\t\trawCount++;\n> > > -\t\t\trawSize = cfg.size;\n> > > +\t\t\trawSize.expandTo(cfg.size);\n> >\n> > We support a single raw stream only, is this change needed ?\n> \n> It makes the two branches look the same\n> \n> > > \t\t} else {\n> > > \t\t\tyuvCount++;\n> > > \t\t\tmaxYuvSize.expandTo(cfg.size);\n> > > @@ -269,24 +278,20 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > \t/*\n> > > \t * Generate raw configuration from CIO2.\n> > > \t *\n> > > -\t * \\todo The image sensor frame size should be selected to optimize\n> > > -\t * operations based on the sizes of the requested streams. However such\n> > > -\t * a selection makes the pipeline configuration procedure fail for small\n> > > -\t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> > > -\t * operations to stall for some stream size combinations (see the\n> > > -\t * commit message of the patch that introduced this comment for more\n> > > -\t * failure examples).\n> > > +\t * The output YUV streams will be limited in size to the\n> > > +\t * maximum frame size requested for the RAW stream, if present.\n> >\n> > This could be reflowed.\n> >\n> > > \t *\n> > > -\t * Until the sensor frame size calculation criteria are clarified, when\n> > > -\t * capturing from ImgU always use the largest possible size which\n> > > -\t * guarantees better results at the expense of the frame rate and CSI-2\n> > > -\t * bus bandwidth. When only a raw stream is requested use the requested\n> > > -\t * size instead, as the ImgU is not involved.\n> > > +\t * If no raw stream is requested generate a size as large as the maximum\n> > > +\t * requested YUV size aligned to the ImgU constraints and bound by the\n> > > +\t * sensor's maximum resolution. See\n> > > +\t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> > > \t */\n> > > -\tif (!yuvCount)\n> > > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > -\telse\n> > > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration({});\n> > > +\tif (rawSize.isNull())\n> > > +\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n> >\n> > Is alignedUpTo() the right function ? It will round up the height to a\n> > multiple of 540, that doesn't sound right.\n> >\n> \n> No it should probably be expandedTo()\n> \n> > > +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> >\n> > Same here.\n> \n> While I guess this one is correct as we need to align up to the\n> margins\n\nAre margins alignments, or a number of pixels to be added because they\nwill be cropped by the processing blocks ?\n\n> > > +\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n> > > +\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > \tif (!cio2Configuration_.pixelFormat.isValid())\n> > > \t\treturn Invalid;\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 84F11BDC71\n\tfor ;\n\tMon, 11 Oct 2021 14:08:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB14B68F4C;\n\tMon, 11 Oct 2021 16:08:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCEFB6012B\n\tfor ;\n\tMon, 11 Oct 2021 16:08:39 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 628112A8;\n\tMon, 11 Oct 2021 16:08:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"osjQbIWw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633961319;\n\tbh=WRWDngjLAkcrc5bt/W0k6LRPJyHUukyv89Zk5YNnDtQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=osjQbIWwlvGS0MIqMkwmlW1jwTmNbjuaCM7sstfbhuoWh+Qw0EDfIhETBseo3lMNQ\n\tu3S2Qlcz9/D6M6vGTuYFA2+4R+B+/QkXTUi0NJtvl+AsK+HEIlmztjcPPOkLV4xRvI\n\tEFLe9xZgn7qTRRf6a1UlpAY/+SAypu/DYk//u2n0=","Date":"Mon, 11 Oct 2021 17:08:26 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-2-jacopo@jmondi.org>\n\t\n\t<20211011081817.vupfxih3lfb6jl7o@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211011081817.vupfxih3lfb6jl7o@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20117,"web_url":"https://patchwork.libcamera.org/comment/20117/","msgid":"<20211011150040.3fkb4ocvdhzsnzfv@uno.localdomain>","date":"2021-10-11T15:00:40","subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Oct 11, 2021 at 05:08:26PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n>\n> On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote:\n> > On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > (Resending with Umang's correct e-mail address now)\n> > >\n> > > On Tue, Sep 07, 2021 at 09:40:51PM +0200, Jacopo Mondi wrote:\n> > > > As reported by commit 7208e70211a6 (\"libcamera: ipu3: Always use sensor\n> > > > full frame size\") the current implementation of the IPU3 pipeline\n> > > > handler always uses the sensor resolution as the ImgU input frame size in\n> > > > order to work around an issue with the ImgU configuration procedure.\n> > > >\n> > > > Now that the frame selection policy has been modified in the CIO2Device\n> > > > class implementation to comply with the requirements of the ImgU\n> > > > configuration script we can remove the workaround and select the most\n> > > > opportune sensor size to feed the ImgU with.\n> > >\n> > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that\n> > > belong to ipu3.cpp instead ?\n> >\n> > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++\n>\n> I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ?\n>\n\nAre you suggesting to move them here ?\n\n> > > > Signed-off-by: Jacopo Mondi \n> > > > Reviewed-by: Paul Elder \n> > > > Reviewed-by: Umang Jain \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------\n> > > > 1 file changed, 23 insertions(+), 18 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index c287bf86e79a..291338288685 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > \t\tstatus = Adjusted;\n> > > > \t}\n> > > >\n> > > > -\t/* Validate the requested stream configuration */\n> > > > +\t/*\n> > > > +\t * Validate the requested stream configuration and select the sensor\n> > > > +\t * format by collecting the maximum RAW stream width and height and\n> > > > +\t * picking the closest larger match.\n> > > > +\t *\n> > > > +\t * If no RAW stream is requested use the one of the largest YUV stream,\n> > > > +\t * plus margin pixels for the IF and BDS rectangle to downscale.\n> > > > +\t *\n> > > > +\t * \\todo Clarify the IF and BDS margins requirements.\n> > > > +\t */\n> > > > \tunsigned int rawCount = 0;\n> > > > \tunsigned int yuvCount = 0;\n> > > > \tSize maxYuvSize;\n> > > > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > >\n> > > > \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > > > \t\t\trawCount++;\n> > > > -\t\t\trawSize = cfg.size;\n> > > > +\t\t\trawSize.expandTo(cfg.size);\n> > >\n> > > We support a single raw stream only, is this change needed ?\n> >\n> > It makes the two branches look the same\n> >\n> > > > \t\t} else {\n> > > > \t\t\tyuvCount++;\n> > > > \t\t\tmaxYuvSize.expandTo(cfg.size);\n> > > > @@ -269,24 +278,20 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > \t/*\n> > > > \t * Generate raw configuration from CIO2.\n> > > > \t *\n> > > > -\t * \\todo The image sensor frame size should be selected to optimize\n> > > > -\t * operations based on the sizes of the requested streams. However such\n> > > > -\t * a selection makes the pipeline configuration procedure fail for small\n> > > > -\t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> > > > -\t * operations to stall for some stream size combinations (see the\n> > > > -\t * commit message of the patch that introduced this comment for more\n> > > > -\t * failure examples).\n> > > > +\t * The output YUV streams will be limited in size to the\n> > > > +\t * maximum frame size requested for the RAW stream, if present.\n> > >\n> > > This could be reflowed.\n> > >\n> > > > \t *\n> > > > -\t * Until the sensor frame size calculation criteria are clarified, when\n> > > > -\t * capturing from ImgU always use the largest possible size which\n> > > > -\t * guarantees better results at the expense of the frame rate and CSI-2\n> > > > -\t * bus bandwidth. When only a raw stream is requested use the requested\n> > > > -\t * size instead, as the ImgU is not involved.\n> > > > +\t * If no raw stream is requested generate a size as large as the maximum\n> > > > +\t * requested YUV size aligned to the ImgU constraints and bound by the\n> > > > +\t * sensor's maximum resolution. See\n> > > > +\t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> > > > \t */\n> > > > -\tif (!yuvCount)\n> > > > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > > -\telse\n> > > > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration({});\n> > > > +\tif (rawSize.isNull())\n> > > > +\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n> > >\n> > > Is alignedUpTo() the right function ? It will round up the height to a\n> > > multiple of 540, that doesn't sound right.\n> > >\n> >\n> > No it should probably be expandedTo()\n> >\n> > > > +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > > +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> > >\n> > > Same here.\n> >\n> > While I guess this one is correct as we need to align up to the\n> > margins\n>\n> Are margins alignments, or a number of pixels to be added because they\n> will be cropped by the processing blocks ?\n>\n\nI wish I knew.\n\nWe never had any real understanding of how to get from a raw size to a\nsuitable output size and what are the requirements of the ImgU in\nterms of processing margins.\n\nThis one works, I would rather not touch it (I don't like operating in\nsuch conservative way because of a lack of understanding, but I have\nno other options here).\n\nThanks\n j\n\n> > > > +\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n> > > > +\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > > \tif (!cio2Configuration_.pixelFormat.isValid())\n> > > > \t\treturn Invalid;\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA6DDC323E\n\tfor ;\n\tMon, 11 Oct 2021 14:59:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A44D68F4C;\n\tMon, 11 Oct 2021 16:59:53 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B09426012B\n\tfor ;\n\tMon, 11 Oct 2021 16:59:51 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id DD5F160002;\n\tMon, 11 Oct 2021 14:59:50 +0000 (UTC)"],"Date":"Mon, 11 Oct 2021 17:00:40 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211011150040.3fkb4ocvdhzsnzfv@uno.localdomain>","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-2-jacopo@jmondi.org>\n\t\n\t<20211011081817.vupfxih3lfb6jl7o@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20120,"web_url":"https://patchwork.libcamera.org/comment/20120/","msgid":"","date":"2021-10-11T15:20:41","subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Oct 11, 2021 at 05:00:40PM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 11, 2021 at 05:08:26PM +0300, Laurent Pinchart wrote:\n> > On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote:\n> > > On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > (Resending with Umang's correct e-mail address now)\n> > > >\n> > > > On Tue, Sep 07, 2021 at 09:40:51PM +0200, Jacopo Mondi wrote:\n> > > > > As reported by commit 7208e70211a6 (\"libcamera: ipu3: Always use sensor\n> > > > > full frame size\") the current implementation of the IPU3 pipeline\n> > > > > handler always uses the sensor resolution as the ImgU input frame size in\n> > > > > order to work around an issue with the ImgU configuration procedure.\n> > > > >\n> > > > > Now that the frame selection policy has been modified in the CIO2Device\n> > > > > class implementation to comply with the requirements of the ImgU\n> > > > > configuration script we can remove the workaround and select the most\n> > > > > opportune sensor size to feed the ImgU with.\n> > > >\n> > > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that\n> > > > belong to ipu3.cpp instead ?\n> > >\n> > > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat\n> > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++\n> >\n> > I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ?\n> \n> Are you suggesting to move them here ?\n\nYes, but not as part of this series. I wanted your opinion on whether\nthis would be a good idea or not. CIO2Device::getSensorFormat()\nimplements constraints coming from the ImgU, which seems to be a bit of\na layering violation to me.\n\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > Reviewed-by: Paul Elder \n> > > > > Reviewed-by: Umang Jain \n> > > > > ---\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------\n> > > > > 1 file changed, 23 insertions(+), 18 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > index c287bf86e79a..291338288685 100644\n> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > > \t\tstatus = Adjusted;\n> > > > > \t}\n> > > > >\n> > > > > -\t/* Validate the requested stream configuration */\n> > > > > +\t/*\n> > > > > +\t * Validate the requested stream configuration and select the sensor\n> > > > > +\t * format by collecting the maximum RAW stream width and height and\n> > > > > +\t * picking the closest larger match.\n> > > > > +\t *\n> > > > > +\t * If no RAW stream is requested use the one of the largest YUV stream,\n> > > > > +\t * plus margin pixels for the IF and BDS rectangle to downscale.\n> > > > > +\t *\n> > > > > +\t * \\todo Clarify the IF and BDS margins requirements.\n> > > > > +\t */\n> > > > > \tunsigned int rawCount = 0;\n> > > > > \tunsigned int yuvCount = 0;\n> > > > > \tSize maxYuvSize;\n> > > > > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > >\n> > > > > \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > > > > \t\t\trawCount++;\n> > > > > -\t\t\trawSize = cfg.size;\n> > > > > +\t\t\trawSize.expandTo(cfg.size);\n> > > >\n> > > > We support a single raw stream only, is this change needed ?\n> > >\n> > > It makes the two branches look the same\n> > >\n> > > > > \t\t} else {\n> > > > > \t\t\tyuvCount++;\n> > > > > \t\t\tmaxYuvSize.expandTo(cfg.size);\n> > > > > @@ -269,24 +278,20 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > > \t/*\n> > > > > \t * Generate raw configuration from CIO2.\n> > > > > \t *\n> > > > > -\t * \\todo The image sensor frame size should be selected to optimize\n> > > > > -\t * operations based on the sizes of the requested streams. However such\n> > > > > -\t * a selection makes the pipeline configuration procedure fail for small\n> > > > > -\t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> > > > > -\t * operations to stall for some stream size combinations (see the\n> > > > > -\t * commit message of the patch that introduced this comment for more\n> > > > > -\t * failure examples).\n> > > > > +\t * The output YUV streams will be limited in size to the\n> > > > > +\t * maximum frame size requested for the RAW stream, if present.\n> > > >\n> > > > This could be reflowed.\n> > > >\n> > > > > \t *\n> > > > > -\t * Until the sensor frame size calculation criteria are clarified, when\n> > > > > -\t * capturing from ImgU always use the largest possible size which\n> > > > > -\t * guarantees better results at the expense of the frame rate and CSI-2\n> > > > > -\t * bus bandwidth. When only a raw stream is requested use the requested\n> > > > > -\t * size instead, as the ImgU is not involved.\n> > > > > +\t * If no raw stream is requested generate a size as large as the maximum\n> > > > > +\t * requested YUV size aligned to the ImgU constraints and bound by the\n> > > > > +\t * sensor's maximum resolution. See\n> > > > > +\t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> > > > > \t */\n> > > > > -\tif (!yuvCount)\n> > > > > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > > > -\telse\n> > > > > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration({});\n> > > > > +\tif (rawSize.isNull())\n> > > > > +\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n> > > >\n> > > > Is alignedUpTo() the right function ? It will round up the height to a\n> > > > multiple of 540, that doesn't sound right.\n> > > >\n> > >\n> > > No it should probably be expandedTo()\n> > >\n> > > > > +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > > > +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> > > >\n> > > > Same here.\n> > >\n> > > While I guess this one is correct as we need to align up to the\n> > > margins\n> >\n> > Are margins alignments, or a number of pixels to be added because they\n> > will be cropped by the processing blocks ?\n> \n> I wish I knew.\n> \n> We never had any real understanding of how to get from a raw size to a\n> suitable output size and what are the requirements of the ImgU in\n> terms of processing margins.\n> \n> This one works, I would rather not touch it\n\nBut this is new code, right ? Do you mean you'd rather not alter the\nbehaviour of the existing implementation in the libcamera master branch,\nor the behaviour of this patch ?\n\n> (I don't like operating in\n> such conservative way because of a lack of understanding, but I have\n> no other options here).\n\nI still have this on my todo list, not to prove you wrong as such, but\nbecause it bothers me too :-)\n\n> > > > > +\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n> > > > > +\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > > > \tif (!cio2Configuration_.pixelFormat.isValid())\n> > > > > \t\treturn Invalid;\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 23E9DBDC71\n\tfor ;\n\tMon, 11 Oct 2021 15:20:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8ECA68F4C;\n\tMon, 11 Oct 2021 17:20:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F0E46012B\n\tfor ;\n\tMon, 11 Oct 2021 17:20:54 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EF9AE8F0;\n\tMon, 11 Oct 2021 17:20:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bpq2JGx7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633965654;\n\tbh=gyBJ4rL0SOmEHuqpO+VYVtPgICqQKDg9ovcnMiL+TtY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bpq2JGx76ha8RCQP5O5XXpO7UggVc2B4llE3lijI9bF4c3UGwnmx3rDxlsY5ATq+D\n\tmmojLuagTpj9g+55TvHXPI/rh/BxCC7rXIFRq81FGJhQvEnovl1qy8Ieiy1+bPw6ks\n\t1FhB0VTgmXX1BM8Aew9i8P5z8molJmgEHrwnH9Ek=","Date":"Mon, 11 Oct 2021 18:20:41 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-2-jacopo@jmondi.org>\n\t\n\t<20211011081817.vupfxih3lfb6jl7o@uno.localdomain>\n\t\n\t<20211011150040.3fkb4ocvdhzsnzfv@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211011150040.3fkb4ocvdhzsnzfv@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20121,"web_url":"https://patchwork.libcamera.org/comment/20121/","msgid":"<20211011153007.xn5yrlzv3eugbkjf@uno.localdomain>","date":"2021-10-11T15:30:07","subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Oct 11, 2021 at 06:20:41PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Oct 11, 2021 at 05:00:40PM +0200, Jacopo Mondi wrote:\n> > On Mon, Oct 11, 2021 at 05:08:26PM +0300, Laurent Pinchart wrote:\n> > > On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote:\n> > > > On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Thank you for the patch.\n> > > > >\n> > > > > (Resending with Umang's correct e-mail address now)\n> > > > >\n> > > > > On Tue, Sep 07, 2021 at 09:40:51PM +0200, Jacopo Mondi wrote:\n> > > > > > As reported by commit 7208e70211a6 (\"libcamera: ipu3: Always use sensor\n> > > > > > full frame size\") the current implementation of the IPU3 pipeline\n> > > > > > handler always uses the sensor resolution as the ImgU input frame size in\n> > > > > > order to work around an issue with the ImgU configuration procedure.\n> > > > > >\n> > > > > > Now that the frame selection policy has been modified in the CIO2Device\n> > > > > > class implementation to comply with the requirements of the ImgU\n> > > > > > configuration script we can remove the workaround and select the most\n> > > > > > opportune sensor size to feed the ImgU with.\n> > > > >\n> > > > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that\n> > > > > belong to ipu3.cpp instead ?\n> > > >\n> > > > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat\n> > > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++\n> > >\n> > > I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ?\n> >\n> > Are you suggesting to move them here ?\n>\n> Yes, but not as part of this series. I wanted your opinion on whether\n> this would be a good idea or not. CIO2Device::getSensorFormat()\n> implements constraints coming from the ImgU, which seems to be a bit of\n> a layering violation to me.\n>\n\nmmmm, dunno... it is conceptually a requirement of the ImgU, but the\nwhole sensor handling is done by the CIO2Device class and hidden from\nthe rest (something I was never really found of). So in both cases we\nviolates a layer. Not sure to be honest\n\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > Reviewed-by: Paul Elder \n> > > > > > Reviewed-by: Umang Jain \n> > > > > > ---\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------\n> > > > > > 1 file changed, 23 insertions(+), 18 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > index c287bf86e79a..291338288685 100644\n> > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > > > \t\tstatus = Adjusted;\n> > > > > > \t}\n> > > > > >\n> > > > > > -\t/* Validate the requested stream configuration */\n> > > > > > +\t/*\n> > > > > > +\t * Validate the requested stream configuration and select the sensor\n> > > > > > +\t * format by collecting the maximum RAW stream width and height and\n> > > > > > +\t * picking the closest larger match.\n> > > > > > +\t *\n> > > > > > +\t * If no RAW stream is requested use the one of the largest YUV stream,\n> > > > > > +\t * plus margin pixels for the IF and BDS rectangle to downscale.\n> > > > > > +\t *\n> > > > > > +\t * \\todo Clarify the IF and BDS margins requirements.\n> > > > > > +\t */\n> > > > > > \tunsigned int rawCount = 0;\n> > > > > > \tunsigned int yuvCount = 0;\n> > > > > > \tSize maxYuvSize;\n> > > > > > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > > >\n> > > > > > \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > > > > > \t\t\trawCount++;\n> > > > > > -\t\t\trawSize = cfg.size;\n> > > > > > +\t\t\trawSize.expandTo(cfg.size);\n> > > > >\n> > > > > We support a single raw stream only, is this change needed ?\n> > > >\n> > > > It makes the two branches look the same\n> > > >\n> > > > > > \t\t} else {\n> > > > > > \t\t\tyuvCount++;\n> > > > > > \t\t\tmaxYuvSize.expandTo(cfg.size);\n> > > > > > @@ -269,24 +278,20 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > > > \t/*\n> > > > > > \t * Generate raw configuration from CIO2.\n> > > > > > \t *\n> > > > > > -\t * \\todo The image sensor frame size should be selected to optimize\n> > > > > > -\t * operations based on the sizes of the requested streams. However such\n> > > > > > -\t * a selection makes the pipeline configuration procedure fail for small\n> > > > > > -\t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> > > > > > -\t * operations to stall for some stream size combinations (see the\n> > > > > > -\t * commit message of the patch that introduced this comment for more\n> > > > > > -\t * failure examples).\n> > > > > > +\t * The output YUV streams will be limited in size to the\n> > > > > > +\t * maximum frame size requested for the RAW stream, if present.\n> > > > >\n> > > > > This could be reflowed.\n> > > > >\n> > > > > > \t *\n> > > > > > -\t * Until the sensor frame size calculation criteria are clarified, when\n> > > > > > -\t * capturing from ImgU always use the largest possible size which\n> > > > > > -\t * guarantees better results at the expense of the frame rate and CSI-2\n> > > > > > -\t * bus bandwidth. When only a raw stream is requested use the requested\n> > > > > > -\t * size instead, as the ImgU is not involved.\n> > > > > > +\t * If no raw stream is requested generate a size as large as the maximum\n> > > > > > +\t * requested YUV size aligned to the ImgU constraints and bound by the\n> > > > > > +\t * sensor's maximum resolution. See\n> > > > > > +\t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> > > > > > \t */\n> > > > > > -\tif (!yuvCount)\n> > > > > > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > > > > -\telse\n> > > > > > -\t\tcio2Configuration_ = data_->cio2_.generateConfiguration({});\n> > > > > > +\tif (rawSize.isNull())\n> > > > > > +\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n> > > > >\n> > > > > Is alignedUpTo() the right function ? It will round up the height to a\n> > > > > multiple of 540, that doesn't sound right.\n> > > > >\n> > > >\n> > > > No it should probably be expandedTo()\n> > > >\n> > > > > > +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > > > > +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> > > > >\n> > > > > Same here.\n> > > >\n> > > > While I guess this one is correct as we need to align up to the\n> > > > margins\n> > >\n> > > Are margins alignments, or a number of pixels to be added because they\n> > > will be cropped by the processing blocks ?\n> >\n> > I wish I knew.\n> >\n> > We never had any real understanding of how to get from a raw size to a\n> > suitable output size and what are the requirements of the ImgU in\n> > terms of processing margins.\n> >\n> > This one works, I would rather not touch it\n>\n> But this is new code, right ? Do you mean you'd rather not alter the\n> behaviour of the existing implementation in the libcamera master branch,\n> or the behaviour of this patch ?\n>\n\nCorrect, and now that I look at how margins are used I could take a\nbet and use instead the defines we have for alignment ?\n\nstatic constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;\nstatic constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n\nstatic constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\nstatic constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n\nTo find out if moving the alignement from 32 to 4 casues any issue.\n\n\n> > (I don't like operating in\n> > such conservative way because of a lack of understanding, but I have\n> > no other options here).\n>\n> I still have this on my todo list, not to prove you wrong as such, but\n> because it bothers me too :-)\n>\n\nI understand :) We've been dragging this debt for too long, I concur.\n\nThanks\n j\n\n> > > > > > +\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n> > > > > > +\tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > > > > \tif (!cio2Configuration_.pixelFormat.isValid())\n> > > > > > \t\treturn Invalid;\n> > > > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 67D37C323E\n\tfor ;\n\tMon, 11 Oct 2021 15:29:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDDA468F4C;\n\tMon, 11 Oct 2021 17:29:19 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E115B6012B\n\tfor ;\n\tMon, 11 Oct 2021 17:29:18 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 3CB7320008;\n\tMon, 11 Oct 2021 15:29:18 +0000 (UTC)"],"Date":"Mon, 11 Oct 2021 17:30:07 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211011153007.xn5yrlzv3eugbkjf@uno.localdomain>","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-2-jacopo@jmondi.org>\n\t\n\t<20211011081817.vupfxih3lfb6jl7o@uno.localdomain>\n\t\n\t<20211011150040.3fkb4ocvdhzsnzfv@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the\n\toptimal sensor size","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]