[libcamera-devel] [PATCH 01/16] libcamera: ipu3: Use the optimal sensor size

[Jean-Michel Hautbois]

On 03/09/2021 10:43, Jacopo Mondi wrote:
> Hello Jean-Michel
> 
> On Fri, Sep 03, 2021 at 09:00:37AM +0200, Jean-Michel Hautbois wrote:
>> Hi Jacopo,
>>
>> On 27/08/2021 14:07, Jacopo Mondi wrote:
>>> As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor
>>> full frame size") the current implementation of the IPU3 pipeline
>>> handler always use the sensor resolution as the ImgU input frame size in
>>> order to work around an issue with the ImgU configuration procedure.
>>>
>>> Now that the frame selection policy has been modified in the CIO2Device
>>> class implementation to comply with the requirements of the ImgU
>>> configuration script we can remove the workaround and select the most
>>> opportune sensor size to feed the ImgU with.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------
>>>  1 file changed, 19 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index c287bf86e79a..b321c94e9cb0 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>>>  		status = Adjusted;
>>>  	}
>>>
>>> -	/* Validate the requested stream configuration */
>>> +	/*
>>> +	 * 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.
>>> +	 *
>>> +	 * If no RAW stream is requested use the one of the largest YUV stream,
>>> +	 * plus margin pixels for the IF and BDS rectangle to downscale.
>>> +	 *
>>> +	 * \todo Clarify the IF and BDS margins requirements.
>>> +	 */
>>>  	unsigned int rawCount = 0;
>>>  	unsigned int yuvCount = 0;
>>>  	Size maxYuvSize;
>>> @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>>>
>>>  		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
>>>  			rawCount++;
>>> -			rawSize = cfg.size;
>>> +			rawSize.expandTo(cfg.size);
>>>  		} else {
>>>  			yuvCount++;
>>>  			maxYuvSize.expandTo(cfg.size);
>>> @@ -269,24 +278,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>>>  	/*
>>>  	 * Generate raw configuration from CIO2.
>>>  	 *
>>> -	 * \todo The image sensor frame size should be selected to optimize
>>> -	 * operations based on the sizes of the requested streams. However such
>>> -	 * a selection makes the pipeline configuration procedure fail for small
>>> -	 * resolutions (for example: 640x480 with OV5670) and causes the capture
>>> -	 * operations to stall for some stream size combinations (see the
>>> -	 * commit message of the patch that introduced this comment for more
>>> -	 * failure examples).
>>> -	 *
>>> -	 * 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.
>>> +	 * The output YUV streams will be limited in size to the
>>> +	 * maximum frame size requested for the RAW stream.
>>>  	 */
>>> -	if (!yuvCount)
>>> -		cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
>>> -	else
>>> -		cio2Configuration_ = data_->cio2_.generateConfiguration({});
>>> +	if (rawSize.isNull())
>>> +		rawSize = maxYuvSize.alignedUpTo(40, 540)
>>> +				    .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
>>> +						 IMGU_OUTPUT_HEIGHT_MARGIN)
>>
>> We have mixed magical values and defined ones... What are 40 and 540
>> exactly ?
>>
> 
> You are right, 40 and 540 are magical :)
> 
> They are defined in imgu.cpp as
> 
> static constexpr unsigned int IF_CROP_MAX_W = 40;
> static constexpr unsigned int IF_CROP_MAX_H = 540;
> 
> And represent the IF rectangle crop limits.
> 
> As the ImgU configuration procedure is iterative, it tries to to find
> a suitable configuration by testing one after the other IF rectangles
> scaled down by a known factor, up to the maximum scaling limits
> expressed above.
> 
> 	/* Populate the configurations vector by scaling width and height. */
> 	unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
> 	unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);
> 	unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
> 	unsigned int minIfHeight = in.height - IF_CROP_MAX_H;
> 	while (ifWidth >= minIfWidth) {
> 		while (ifHeight >= minIfHeight) {
> 			Size iif{ ifWidth, ifHeight };
> 			calculateBDS(pipe, iif, gdc, sf);
> 			ifHeight -= IF_ALIGN_H;
> 		}
> 
> 		ifWidth -= IF_ALIGN_W;
> 	}
> 
> 	/* Repeat search by scaling width first. */
> 	ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
> 	ifHeight = utils::alignUp(in.height, IF_ALIGN_H);
> 	minIfWidth = in.width - IF_CROP_MAX_W;
> 	minIfHeight = in.height - IF_CROP_MAX_H;
> 	while (ifHeight >= minIfHeight) {
> 		/*
> 		 * \todo This procedure is probably broken:
> 		 * https://github.com/intel/intel-ipu3-pipecfg/issues/2
> 		 */
> 		while (ifWidth >= minIfWidth) {
> 			Size iif{ ifWidth, ifHeight };
> 			calculateBDS(pipe, iif, gdc, sf);
> 			ifWidth -= IF_ALIGN_W;
> 		}
> 
> 		ifHeight -= IF_ALIGN_H;
> 	}
> 
> For this reason, we refuse input sizes smaller than the IF scaling
> limits
> 
> 	/*
> 	 * \todo Filter out all resolutions < IF_CROP_MAX.
> 	 * See https://bugs.libcamera.org/show_bug.cgi?id=32
> 	 */
> 	if (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) {
> 		LOG(IPU3, Error) << "Input resolution " << in.toString()
> 				 << " not supported";
> 		return {};
> 	}
> 
> And so here in ipu3.cpp we try to find a raw size from the sensor to
> feed the ImgU with larger than [40, 450].
> 
> I'll move those definitions to imgu.h and use a more expressive
> variable instead of the raw numbers.

Thank you ! Could you also add a reference to the bug for reference maybe ?

> 
> Thanks
>    j
> 
>>> +				    .boundedTo(data_->cio2_.sensor()->resolution());
>>> +	cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
>>>  	if (!cio2Configuration_.pixelFormat.isValid())
>>>  		return Invalid;
>>>
>>>