[libcamera-devel] [PATCH 4/7] libcamera: ipu3: imgu: Fix IF height selection

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 18, 2021 at 11:39:38AM +0100, Jacopo Mondi wrote:
> Apply to calculateBDSHeight() function the first hunk of commit 243d134
> ("Fix some bug for some resolutions") from
> https://github.com/intel/intel-ipu3-pipecfg.git.
> 
> The condition for the computed IF rectangle height to be matched
> against the desired alignment now makes sure that it is included
> in the minimum and maximum acceptable values.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 11bb7cb95084..89a01bddbed2 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
>  		estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);
>  
>  		ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> -		while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {
> +		while (ifHeight >= minIFHeight && ifHeight <= iif.height &&

iif.height isn't changed in the loop, and ifHeight is only decreased.
The new condition can either be false on the first iteration, in which
case the loop will be skipped completely, or it will always be true and
won't have an effect.

> +		       ifHeight / bdsSF >= minBDSHeight) {
>  
>  			bdsHeight = ifHeight / bdsSF;
>  			if (std::fmod(bdsHeight, 1.0) == 0) {
> @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
>  		}
>  
>  		ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> -		while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {
> +		while (ifHeight >= minIFHeight && ifHeight <= iif.height &&

If the condition was false on the first iteration of the previous loop,
if will still be false here, and both loops will be skipped. There's
something fishy in this code. It doesn't seem significantly worse after
this change though.

> +		       ifHeight / bdsSF >= minBDSHeight) {
>  
>  			bdsHeight = ifHeight / bdsSF;
>  			if (std::fmod(bdsHeight, 1.0) == 0) {

-- 
Regards,

Laurent Pinchart