[libcamera-devel] libcamera: ipu3: Fix RAW sizes selection
diff mbox series

Message ID 20210203112316.150393-1-jacopo@jmondi.org
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: ipu3: Fix RAW sizes selection
Related show

Commit Message

Jacopo Mondi Feb. 3, 2021, 11:23 a.m. UTC 
Commit 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame size")
changed the CIO2 configuration procedure to always select the full
sensor resolution to work around an ImgU configuration issue, as
reported in the todo item introduced in said commit.

When capturing a RAW stream only and the ImgU is not involved, the CIO2
has to be configured with the requested stream size to avoid adjusting
the stream to the sensor resolution.

As an example, capturing a raw frame smaller than the sensor resolution
with this patch applied results in the stream configuration to be
correctly assigned.

$ cam -c1 -swidth=1056,height=784,role=raw
ipu3.cpp:207 CIO2 configuration: 1056x784-SGRBG10_IPU3
ipu3.cpp:222 Validating stream: 1056x784-SGRBG10_IPU3
ipu3.cpp:233 Assigned 1056x784-SGRBG10_IPU3 to the raw stream

Without this patch the same operation results in the stream resolution
to be adjusted to the sensor resolution.

$ cam -c1 -swidth=1056,height=784,role=raw
ipu3.cpp:201 CIO2 configuration: 4224x3136-SGRBG10_IPU3
ipu3.cpp:216 Validating stream: 1056x784-SGRBG10_IPU3
ipu3.cpp:227 Assigned 4224x3136-SGRBG10_IPU3 to the raw stream
ipu3.cpp:297 Stream 0 configuration adjusted to 4224x3136-SGRBG10_IPU3
Camera configuration adjusted

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Niklas Söderlund Feb. 4, 2021, 9 a.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2021-02-03 12:23:16 +0100, Jacopo Mondi wrote:
> Commit 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame size")
> changed the CIO2 configuration procedure to always select the full
> sensor resolution to work around an ImgU configuration issue, as
> reported in the todo item introduced in said commit.
> 
> When capturing a RAW stream only and the ImgU is not involved, the CIO2
> has to be configured with the requested stream size to avoid adjusting
> the stream to the sensor resolution.
> 
> As an example, capturing a raw frame smaller than the sensor resolution
> with this patch applied results in the stream configuration to be
> correctly assigned.
> 
> $ cam -c1 -swidth=1056,height=784,role=raw
> ipu3.cpp:207 CIO2 configuration: 1056x784-SGRBG10_IPU3
> ipu3.cpp:222 Validating stream: 1056x784-SGRBG10_IPU3
> ipu3.cpp:233 Assigned 1056x784-SGRBG10_IPU3 to the raw stream
> 
> Without this patch the same operation results in the stream resolution
> to be adjusted to the sensor resolution.
> 
> $ cam -c1 -swidth=1056,height=784,role=raw
> ipu3.cpp:201 CIO2 configuration: 4224x3136-SGRBG10_IPU3
> ipu3.cpp:216 Validating stream: 1056x784-SGRBG10_IPU3
> ipu3.cpp:227 Assigned 4224x3136-SGRBG10_IPU3 to the raw stream
> ipu3.cpp:297 Stream 0 configuration adjusted to 4224x3136-SGRBG10_IPU3
> Camera configuration adjusted
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index db0d6b91be70..d73dd50e2f1d 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -162,12 +162,14 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	unsigned int rawCount = 0;
>  	unsigned int yuvCount = 0;
>  	Size maxYuvSize;
> +	Size maxRawSize;
>  
>  	for (const StreamConfiguration &cfg : config_) {
>  		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>  
>  		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
>  			rawCount++;
> +			maxRawSize.expandTo(cfg.size);
>  		} else {
>  			yuvCount++;
>  			maxYuvSize.expandTo(cfg.size);
> @@ -190,11 +192,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	 * commit message of the patch that introduced this comment for more
>  	 * failure examples).
>  	 *
> -	 * Until the sensor frame size calculation criteria are clarified,
> -	 * always use the largest possible one which guarantees better results
> -	 * at the expense of the frame rate and CSI-2 bus bandwidth.
> +	 * Until the sensor frame size calculation criteria are clarified, when
> +	 * capturing from ImgU always use the largest possible size which
> +	 * guarantees better results at the expense of the frame rate and CSI-2
> +	 * bus bandwidth. When only a raw stream is requested use the requested
> +	 * size instead, as the ImgU is not involved.
>  	 */
> -	cio2Configuration_ = data_->cio2_.generateConfiguration({});
> +	if (!yuvCount)
> +		cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
> +	else
> +		cio2Configuration_ = data_->cio2_.generateConfiguration({});
>  	if (!cio2Configuration_.pixelFormat.isValid())
>  		return Invalid;
>  
> -- 
> 2.30.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Feb. 4, 2021, 11:13 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Wed, Feb 03, 2021 at 12:23:16PM +0100, Jacopo Mondi wrote:
> Commit 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame size")
> changed the CIO2 configuration procedure to always select the full
> sensor resolution to work around an ImgU configuration issue, as
> reported in the todo item introduced in said commit.
> 
> When capturing a RAW stream only and the ImgU is not involved, the CIO2
> has to be configured with the requested stream size to avoid adjusting
> the stream to the sensor resolution.
> 
> As an example, capturing a raw frame smaller than the sensor resolution
> with this patch applied results in the stream configuration to be
> correctly assigned.
> 
> $ cam -c1 -swidth=1056,height=784,role=raw
> ipu3.cpp:207 CIO2 configuration: 1056x784-SGRBG10_IPU3
> ipu3.cpp:222 Validating stream: 1056x784-SGRBG10_IPU3
> ipu3.cpp:233 Assigned 1056x784-SGRBG10_IPU3 to the raw stream
> 
> Without this patch the same operation results in the stream resolution
> to be adjusted to the sensor resolution.
> 
> $ cam -c1 -swidth=1056,height=784,role=raw
> ipu3.cpp:201 CIO2 configuration: 4224x3136-SGRBG10_IPU3
> ipu3.cpp:216 Validating stream: 1056x784-SGRBG10_IPU3
> ipu3.cpp:227 Assigned 4224x3136-SGRBG10_IPU3 to the raw stream
> ipu3.cpp:297 Stream 0 configuration adjusted to 4224x3136-SGRBG10_IPU3
> Camera configuration adjusted
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index db0d6b91be70..d73dd50e2f1d 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -162,12 +162,14 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	unsigned int rawCount = 0;
>  	unsigned int yuvCount = 0;
>  	Size maxYuvSize;
> +	Size maxRawSize;
>  
>  	for (const StreamConfiguration &cfg : config_) {
>  		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>  
>  		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
>  			rawCount++;
> +			maxRawSize.expandTo(cfg.size);

Given that we can't have more than one raw stream you could also write

			maxRawSize = cfg.size;

and possibly event rename the variable to rawSize. Up to you.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		} else {
>  			yuvCount++;
>  			maxYuvSize.expandTo(cfg.size);
> @@ -190,11 +192,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	 * commit message of the patch that introduced this comment for more
>  	 * failure examples).
>  	 *
> -	 * Until the sensor frame size calculation criteria are clarified,
> -	 * always use the largest possible one which guarantees better results
> -	 * at the expense of the frame rate and CSI-2 bus bandwidth.
> +	 * Until the sensor frame size calculation criteria are clarified, when
> +	 * capturing from ImgU always use the largest possible size which
> +	 * guarantees better results at the expense of the frame rate and CSI-2
> +	 * bus bandwidth. When only a raw stream is requested use the requested
> +	 * size instead, as the ImgU is not involved.
>  	 */
> -	cio2Configuration_ = data_->cio2_.generateConfiguration({});
> +	if (!yuvCount)
> +		cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
> +	else
> +		cio2Configuration_ = data_->cio2_.generateConfiguration({});
>  	if (!cio2Configuration_.pixelFormat.isValid())
>  		return Invalid;
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index db0d6b91be70..d73dd50e2f1d 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -162,12 +162,14 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 	unsigned int rawCount = 0;
 	unsigned int yuvCount = 0;
 	Size maxYuvSize;
+	Size maxRawSize;
 
 	for (const StreamConfiguration &cfg : config_) {
 		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
 
 		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
 			rawCount++;
+			maxRawSize.expandTo(cfg.size);
 		} else {
 			yuvCount++;
 			maxYuvSize.expandTo(cfg.size);
@@ -190,11 +192,16 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 	 * commit message of the patch that introduced this comment for more
 	 * failure examples).
 	 *
-	 * Until the sensor frame size calculation criteria are clarified,
-	 * always use the largest possible one which guarantees better results
-	 * at the expense of the frame rate and CSI-2 bus bandwidth.
+	 * Until the sensor frame size calculation criteria are clarified, when
+	 * capturing from ImgU always use the largest possible size which
+	 * guarantees better results at the expense of the frame rate and CSI-2
+	 * bus bandwidth. When only a raw stream is requested use the requested
+	 * size instead, as the ImgU is not involved.
 	 */
-	cio2Configuration_ = data_->cio2_.generateConfiguration({});
+	if (!yuvCount)
+		cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
+	else
+		cio2Configuration_ = data_->cio2_.generateConfiguration({});
 	if (!cio2Configuration_.pixelFormat.isValid())
 		return Invalid;