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

[Jacopo Mondi]

Hi Laurent,

On Tue, Apr 13, 2021 at 02:18:01AM +0300, Laurent Pinchart wrote:
> 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.

Indeed, ifHeight should be re-initialized

I've opened an issue on the script repository to report this
https://github.com/intel/intel-ipu3-pipecfg/issues/2

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