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

[Laurent Pinchart]

Hi Jacopo,

On Wed, Apr 21, 2021 at 03:01:18PM +0200, Jacopo Mondi wrote:
> On Tue, Apr 13, 2021 at 03:14:43AM +0300, Laurent Pinchart wrote:
> > 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

As commented on patch 3/7, I don't think that's a big issue, we
shouldn't duplicate the python code in the first place.

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

-- 
Regards,

Laurent Pinchart