[libcamera-devel] [PATCH] libcamera: ipu3: Always use sensor full frame size

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Sep 17, 2020 at 01:28:39PM +0200, Jacopo Mondi wrote:
> When calculating the pipeline configuration for the IPU3 platform,
> libcamera tries to be smart and select the smallest sensor frame
> resolution larger enough to accommodate the stream sizes

s/larger/large/

> requested by the application.
> 
> While this seems to make a lot of sense, in practice optimizing the

s/seems to make/makes/

> selected sensor resolution makes the pipeline configuration calculation
> process fail in multiple occasions, or results in stalls during capture.
> 
> As a trivial example, capturing with cam with the following command
> line results in a stall:
> $ cam -swidth=1280,height=720 -swidth=640,height=480 -c1 -C
> 
> Likewise, the Android HAL supported format enumeration fails in
> reporting smaller resolutions as supported when used with the OV5670
> sensor.
> 
> 320x240:
> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 648x486-SGRBG10_IPU3
> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration
> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions.
> 
> 640x480:
> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 320x240-SGRBG10_IPU3
> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration
> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions.
> 
> Furthermore the reference xml files used for the IPU3 camera
> configuration on the ChromeOS platform restricts the number of sensor
> resolution to be used for the OV5670 sensor to 2 from the 6 supported by
> the driver [1].
> 
> The selection criteria of the correct CIO2 mode are not specified, and
> for the time being, always use the sensor maximum resolution at the
> expense of frame rate and bus bandwidth to allow the pipeline to successfully
> support smaller modes for the OV5670 sensor and solves pipeline stalls

s/solves/solve/

> when capturing with both sensors.

I would make it clearer this is a temporary workaround. Maybe "For the
time being, as a workaround, ..." ?

> 
> [1] See the <sensor_modes> enumeration in:
> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> RFC->v1:
> - Add Niklas' tag
> - Expand the \todo comment and remove the existing comment block
> 
> Just have a look at the \todo block wording, if no big questions I'll push this
> one soon.
> 
> Thanks
>   j
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 31 ++++++++++++----------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 2d881fe28f98..2ba00a2ca211 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -144,25 +144,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		status = Adjusted;
>  	}
> 
> -	/*
> -	 * Validate the requested stream configuration and select the sensor
> -	 * format by collecting the maximum RAW stream width and height and
> -	 * picking the closest larger match, as the IPU3 can downscale only. If
> -	 * no resolution is requested for the RAW stream, use the one from the
> -	 * largest YUV stream, plus margins pixels for the IF and BDS to scale.
> -	 * If no resolution is requested for any stream, pick the largest one.
> -	 */
> +	/* Validate the requested stream configuration */
>  	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);
> @@ -174,18 +165,22 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		return Invalid;
>  	}
> 
> -	if (maxRawSize.isNull())
> -		maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> -						    IMGU_OUTPUT_HEIGHT_MARGIN)
> -				       .boundedTo(data_->cio2_.sensor()->resolution());
> -
>  	/*
>  	 * Generate raw configuration from CIO2.
>  	 *
> -	 * The output YUV streams will be limited in size to the maximum
> -	 * frame size requested for the RAW stream.
> +	 * \todo The image sensor frame size should be calculated as the
> +	 * smallest possible resolution larger enough to accommodate the
> +	 * requested stream sizes. However such a selection makes the pipeline

Smallest possible size may not always be the best, we haven't really
thought about this. I would write "The image sensor output size should
be selected to optimize operation based on the sizes of the requested
streams.".

> +	 * configuration procedure fail for small resolution (in example:

s/in example/for example/

> +	 * 640x480 with OV5670) and causes the capture operations to stall for
> +	 * some streams size combinations (see the commit message of the patch

s/streams/stream/

> +	 * that introduced this comment for more failure examples).
> +	 *
> +	 * Until the sensor frame size calculation criteria are not clarified,

s/are not clarified/are clarified/

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

> +	 * always use the largest possible one which guarantees better results
> +	 * at the expense of the frame rate and CSI-2 bus bandwidth.
>  	 */
> -	cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
> +	cio2Configuration_ = data_->cio2_.generateConfiguration({});
>  	if (!cio2Configuration_.pixelFormat.isValid())
>  		return Invalid;
> 

-- 
Regards,

Laurent Pinchart