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

[Jacopo Mondi]

Hi Laurent

On Tue, Apr 13, 2021 at 03:14:43AM +0300, Laurent Pinchart wrote:
> 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.

You know, I presume this might even be intended, even if it was
probably better coded as

        if (ifHeight > iif.height) {
                while (ifHeight >= minIFHeight &&
                       ifHeight / bdsSF >= minBDSHeight) {

                        ....
                        ifHeight -= IF_ALIGN_H;
                }

                while (ifHeight >= minIFHeight && ifHeight <= iif.height &&
                       ifHeight / bdsSF >= minBDSHeight) {

                        ....
                        ifHeight += IF_ALIGN_H;

                }
        }

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