[libcamera-devel] [PATCH 1/7] libcamera: ipu3: imgu: Update BDS calculation process

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 18, 2021 at 11:39:35AM +0100, Jacopo Mondi wrote:
> Apply the last three hunks of commit 243d134 ("Fix some bug for some
> resolutions") from https://github.com/intel/intel-ipu3-pipecfg.git
> to the BSD calculation procedure.


s/BSD/BDS/

and below too.

> The BSD calculation is now perfomed by scaling both width and height,
> and repeated by scaling width first.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index d5cf05b0c421..4a1b0a9528f2 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -394,19 +394,39 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
>  	const Size &in = pipe->input;
>  	Size gdc = calculateGDC(pipe);
>  
> -	unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
> -	unsigned int ifHeight = in.height;
> -	unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
>  	float bdsSF = static_cast<float>(in.width) / gdc.width;
>  	float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
>  
> +	/* Populate the configurations vector by scaling widht and height. */

s/widht/width/

> +	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) {
> -		Size iif{ ifWidth, ifHeight };
> -		calculateBDS(pipe, iif, gdc, sf);
> +		while (ifHeight >= minIfHeight) {
> +			Size iif{ ifWidth, ifHeight };
> +			calculateBDS(pipe, iif, gdc, sf);
> +			ifHeight -= IF_ALIGN_H;
> +		}
>  
>  		ifWidth -= IF_ALIGN_W;
>  	}

While this seems to replicate the python code mentioned in the commit
message, the logic seems very strange to me. In the first iteration of
the outer loop, the inner loop will iterate over all ifHeight values
larger than the minimum. The inner loop will exit with ifHeight having
an invalid value. Then the second iteration of the outer loop will try
the next ifWidth value, but the inner loop will be skipped, as ifHeight
will be < minIfHeight already.

I'll reiterate my previous concern: if we merely copy the python code
without understanding the logic, this will always remain broken.

>  
> +	/* 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) {
> +		while (ifWidth >= minIfWidth) {
> +			Size iif{ ifWidth, ifHeight };
> +			calculateBDS(pipe, iif, gdc, sf);
> +			ifWidth -= IF_ALIGN_W;
> +		}
> +
> +		ifHeight -= IF_ALIGN_H;
> +	}
> +
>  	if (pipeConfigs.size() == 0) {
>  		LOG(IPU3, Error) << "Failed to calculate pipe configuration";
>  		return {};

-- 
Regards,

Laurent Pinchart