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

Correct, but in this second loop ifHeight is increased, so the check
is required. I agree in the first loop it could have been a condition
external to the while() but it would be one indentation level that can
be skipped imho.

Again, deviating from the script is risky in terms of eventual backporting

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