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

[Kieran Bingham]

Hi Jacopo,

On 18/09/2020 03:57, Laurent Pinchart wrote:
> 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.".

Indeed, we really need to think about how we present this as an option
to the users/applications as well.

This is /use case/ dependant.

A mobile platform wants to reduce power for instance, and might want the
lowest reasonable sensor size to capture the required results, but other
use-cases (perhaps a digital microscope) would want to use as high a
resolution as possible from the sensor to capture as much light
information as is possible, and deal with any size constraints at the
rescaler.

And of course it can also depend on how the resolution selection affects
the field of view ...

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

s/resolution/resolutions/

> 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.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>>  	 */
>> -	cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
>> +	cio2Configuration_ = data_->cio2_.generateConfiguration({});
>>  	if (!cio2Configuration_.pixelFormat.isValid())
>>  		return Invalid;
>>
> 

-- 
Regards
--
Kieran