[{"id":19191,"web_url":"https://patchwork.libcamera.org/comment/19191/","msgid":"<20210831015027.GB20879@jade.amanokami.net>","date":"2021-08-31T01:50:27","subject":"Re: [libcamera-devel] [PATCH 01/16] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Aug 27, 2021 at 02:07:42PM +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 use the sensor resolution as the ImgU input frame size in\n\ns/use/uses\n\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> Signed-off-by: Jacopo Mondi \n\nLooks good to me.\n\nReviewed-by: Paul Elder \n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------\n> 1 file changed, 19 insertions(+), 19 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..b321c94e9cb0 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> \t\t} else {\n> \t\t\tyuvCount++;\n> \t\t\tmaxYuvSize.expandTo(cfg.size);\n> @@ -269,24 +278,15 @@ 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 *\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 * The output YUV streams will be limited in size to the\n> +\t * maximum frame size requested for the RAW stream.\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> +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\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> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF3BFBD87D\n\tfor ;\n\tTue, 31 Aug 2021 01:50:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CC0D6916A;\n\tTue, 31 Aug 2021 03:50:41 +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 F3D5C68891\n\tfor ;\n\tTue, 31 Aug 2021 03:50:38 +0200 (CEST)","from jade.amanokami.net (KD027085206055.au-net.ne.jp\n\t[27.85.206.55])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A9FB924F;\n\tTue, 31 Aug 2021 03:50:36 +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=\"O2qkdRvq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630374638;\n\tbh=cTKpS5bCIZrxEAVhUEUudtWnO7sZODzcFGWkQ/y7wQk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O2qkdRvqFDQNwFIdJfi1ibiZ3ZPeMSa/caLO95RHkXgHCZj1QgKAVvpH/asaCajts\n\t4g1TXi2C0TmdcNrUW8GzZ3qE3UvnOui3juoA0hS3mPY61g/4y5qLJdoMXOr7V19OWW\n\t/G5j0JSqK57yFwJEvXl3Ongm4Yk48ChQtPi7exDo=","Date":"Tue, 31 Aug 2021 10:50:27 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Message-ID":"<20210831015027.GB20879@jade.amanokami.net>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210827120757.110615-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 01/16] 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":19238,"web_url":"https://patchwork.libcamera.org/comment/19238/","msgid":"<5280e164-c9eb-7c64-8d21-db06955cb9c9@ideasonboard.com>","date":"2021-09-01T06:15:54","subject":"Re: [libcamera-devel] [PATCH 01/16] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 8/27/21 5:37 PM, 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 use 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> Signed-off-by: Jacopo Mondi \n\n\nReviewed-by: Umang Jain \n\n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------\n> 1 file changed, 19 insertions(+), 19 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..b321c94e9cb0 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> \t\t} else {\n> \t\t\tyuvCount++;\n> \t\t\tmaxYuvSize.expandTo(cfg.size);\n> @@ -269,24 +278,15 @@ 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 *\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 * The output YUV streams will be limited in size to the\n> +\t * maximum frame size requested for the RAW stream.\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> +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\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 7611FBDC71\n\tfor ;\n\tWed, 1 Sep 2021 06:16:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBCC46916A;\n\tWed, 1 Sep 2021 08:16:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A50660252\n\tfor ;\n\tWed, 1 Sep 2021 08:16:00 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B090C3D7;\n\tWed, 1 Sep 2021 08:15:58 +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=\"EYkO4Icr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630476959;\n\tbh=g+miAMP7R/aIcK2h24Aita72GvqagwTOeug8oK3XqIw=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=EYkO4IcrUFSvUETnKF6055LKLofsR2x7DeTzgT+Qw8j2r1sUDND1/hKV9XErfoNuT\n\ttpHqEqzTZ3YvfhVC42aeu6X35vf9ciFNU5WAZISiGALmnKcNj8rmVCEpPSO/oAZ8g+\n\t+4CbTg7176C11nxvzXk/kyklWQLgi2j3TosCqH1E=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-2-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"<5280e164-c9eb-7c64-8d21-db06955cb9c9@ideasonboard.com>","Date":"Wed, 1 Sep 2021 11:45:54 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210827120757.110615-2-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 01/16] 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19316,"web_url":"https://patchwork.libcamera.org/comment/19316/","msgid":"","date":"2021-09-03T07:00:37","subject":"Re: [libcamera-devel] [PATCH 01/16] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 27/08/2021 14:07, 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 use 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> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------\n> 1 file changed, 19 insertions(+), 19 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..b321c94e9cb0 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> \t\t} else {\n> \t\t\tyuvCount++;\n> \t\t\tmaxYuvSize.expandTo(cfg.size);\n> @@ -269,24 +278,15 @@ 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 *\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 * The output YUV streams will be limited in size to the\n> +\t * maximum frame size requested for the RAW stream.\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> +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n\nWe have mixed magical values and defined ones... What are 40 and 540\nexactly ?\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>","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 03EC6BD87D\n\tfor ;\n\tFri, 3 Sep 2021 07:00:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58F526916A;\n\tFri, 3 Sep 2021 09:00:42 +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 5CDE469165\n\tfor ;\n\tFri, 3 Sep 2021 09:00:40 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:8eca:a50c:50a1:df40])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ECE00BBE;\n\tFri, 3 Sep 2021 09:00:39 +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=\"EKtEc/Bh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630652440;\n\tbh=zhvzH4u2diPGPuYkkKT5+ARZqS8GuHk4P8qLrzYgNd4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=EKtEc/BhXXhosQg0rzgGm8LV4HWhcwzIIE0f415HlsJhajAvFgW24uVUu59T4Skt5\n\t2pi1W1w99K0fENwSsJtIK0Qxb/8q+4Vgl3i7FqPc9SWw63AUCAfctPThSGxsU8YZbX\n\t+hChDqbvutWyQGaYtrxHGj+u+WDZA5TaouE+ziBI=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-2-jacopo@jmondi.org>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Fri, 3 Sep 2021 09:00:37 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<20210827120757.110615-2-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 01/16] 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19327,"web_url":"https://patchwork.libcamera.org/comment/19327/","msgid":"<20210903084350.akvmvyvyqsgtok5r@uno.localdomain>","date":"2021-09-03T08:43:50","subject":"Re: [libcamera-devel] [PATCH 01/16] 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":"Hello Jean-Michel\n\nOn Fri, Sep 03, 2021 at 09:00:37AM +0200, Jean-Michel Hautbois wrote:\n> Hi Jacopo,\n>\n> On 27/08/2021 14:07, 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 use 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> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------\n> > 1 file changed, 19 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index c287bf86e79a..b321c94e9cb0 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> > \t\t} else {\n> > \t\t\tyuvCount++;\n> > \t\t\tmaxYuvSize.expandTo(cfg.size);\n> > @@ -269,24 +278,15 @@ 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 *\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 * The output YUV streams will be limited in size to the\n> > +\t * maximum frame size requested for the RAW stream.\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> > +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n>\n> We have mixed magical values and defined ones... What are 40 and 540\n> exactly ?\n>\n\nYou are right, 40 and 540 are magical :)\n\nThey are defined in imgu.cpp as\n\nstatic constexpr unsigned int IF_CROP_MAX_W = 40;\nstatic constexpr unsigned int IF_CROP_MAX_H = 540;\n\nAnd represent the IF rectangle crop limits.\n\nAs the ImgU configuration procedure is iterative, it tries to to find\na suitable configuration by testing one after the other IF rectangles\nscaled down by a known factor, up to the maximum scaling limits\nexpressed above.\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\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\nFor this reason, we refuse input sizes smaller than the IF scaling\nlimits\n\n\t/*\n\t * \\todo Filter out all resolutions < IF_CROP_MAX.\n\t * See https://bugs.libcamera.org/show_bug.cgi?id=32\n\t */\n\tif (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) {\n\t\tLOG(IPU3, Error) << \"Input resolution \" << in.toString()\n\t\t\t\t << \" not supported\";\n\t\treturn {};\n\t}\n\nAnd so here in ipu3.cpp we try to find a raw size from the sensor to\nfeed the ImgU with larger than [40, 450].\n\nI'll move those definitions to imgu.h and use a more expressive\nvariable instead of the raw numbers.\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> >","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 1FE0EBD87D\n\tfor ;\n\tFri, 3 Sep 2021 08:43:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 846D26916B;\n\tFri, 3 Sep 2021 10:43:03 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE33169165\n\tfor ;\n\tFri, 3 Sep 2021 10:43:02 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 40E8D240005;\n\tFri, 3 Sep 2021 08:43:02 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 10:43:50 +0200","From":"Jacopo Mondi ","To":"Jean-Michel Hautbois ","Message-ID":"<20210903084350.akvmvyvyqsgtok5r@uno.localdomain>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-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 01/16] 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":19331,"web_url":"https://patchwork.libcamera.org/comment/19331/","msgid":"<2d590215-eeda-733b-aade-667b526a67ae@ideasonboard.com>","date":"2021-09-03T08:51:35","subject":"Re: [libcamera-devel] [PATCH 01/16] libcamera: ipu3: Use the\n\toptimal sensor size","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 03/09/2021 10:43, Jacopo Mondi wrote:\n> Hello Jean-Michel\n> \n> On Fri, Sep 03, 2021 at 09:00:37AM +0200, Jean-Michel Hautbois wrote:\n>> Hi Jacopo,\n>>\n>> On 27/08/2021 14:07, 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 use 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>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------\n>>> 1 file changed, 19 insertions(+), 19 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index c287bf86e79a..b321c94e9cb0 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>>> \t\t} else {\n>>> \t\t\tyuvCount++;\n>>> \t\t\tmaxYuvSize.expandTo(cfg.size);\n>>> @@ -269,24 +278,15 @@ 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 *\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 * The output YUV streams will be limited in size to the\n>>> +\t * maximum frame size requested for the RAW stream.\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>>> +\t\t\t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n>>> +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n>>\n>> We have mixed magical values and defined ones... What are 40 and 540\n>> exactly ?\n>>\n> \n> You are right, 40 and 540 are magical :)\n> \n> They are defined in imgu.cpp as\n> \n> static constexpr unsigned int IF_CROP_MAX_W = 40;\n> static constexpr unsigned int IF_CROP_MAX_H = 540;\n> \n> And represent the IF rectangle crop limits.\n> \n> As the ImgU configuration procedure is iterative, it tries to to find\n> a suitable configuration by testing one after the other IF rectangles\n> scaled down by a known factor, up to the maximum scaling limits\n> expressed above.\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\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> For this reason, we refuse input sizes smaller than the IF scaling\n> limits\n> \n> \t/*\n> \t * \\todo Filter out all resolutions < IF_CROP_MAX.\n> \t * See https://bugs.libcamera.org/show_bug.cgi?id=32\n> \t */\n> \tif (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) {\n> \t\tLOG(IPU3, Error) << \"Input resolution \" << in.toString()\n> \t\t\t\t << \" not supported\";\n> \t\treturn {};\n> \t}\n> \n> And so here in ipu3.cpp we try to find a raw size from the sensor to\n> feed the ImgU with larger than [40, 450].\n> \n> I'll move those definitions to imgu.h and use a more expressive\n> variable instead of the raw numbers.\n\nThank you ! Could you also add a reference to the bug for reference maybe ?\n\n> \n> Thanks\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>>>","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 A2B7BC3241\n\tfor ;\n\tFri, 3 Sep 2021 08:51:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C54A69171;\n\tFri, 3 Sep 2021 10:51:39 +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 8CF5169165\n\tfor ;\n\tFri, 3 Sep 2021 10:51:37 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:8eca:a50c:50a1:df40])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38622BBE;\n\tFri, 3 Sep 2021 10:51:37 +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=\"kf65N/Fq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630659097;\n\tbh=lwJCSuWgZdaWw5ZL+Lx8KbcPYfbpFkMJyW9A5dGn/ZA=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=kf65N/FqUfP8N1mr3kdC3/WtrIh35FV2s4/Z0cHsEaGmFBlbArbc/TE9xZzwaysnp\n\tBGvW073/O6Wib35s1Gf4sGTWI/9wSZPfnWkZSjfpBfAQAcw6tpzBqzHnYIm36UMM+W\n\tqwNOF8Cnl5kDMsbOM+9CXSLInbuR+rWCBsTdaWZQ=","To":"Jacopo Mondi ","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-2-jacopo@jmondi.org>\n\t\n\t<20210903084350.akvmvyvyqsgtok5r@uno.localdomain>","From":"Jean-Michel Hautbois ","Message-ID":"<2d590215-eeda-733b-aade-667b526a67ae@ideasonboard.com>","Date":"Fri, 3 Sep 2021 10:51:35 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<20210903084350.akvmvyvyqsgtok5r@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 01/16] 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\" "}}]