[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.
>
> I'll reiterate my previous concern: if we merely copy the python code
> without understanding the logic, this will always remain broken.
>

I'm sorry but all I can do, without any documentation and with just
the python script as information source about the ImgU pipe
configuration procedure is to make sure we stay in sync with the most
recent script version and that the two behaves the same. This part
would have to be rewritten as it is embarassing, but I'm a bit
skeptical about distancing ourselves from the tool, as backporting
fixes will become very hard.

I'm fine dropping this series if there's a plan to re-implement the
pipe configuration.

> >
> > +	/* 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