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

[Laurent Pinchart]

Hi Jacopo,

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

We have various tools at our disposal, one of them being
reverse-engineering :-) I don't agree that there's nothing we can do,
but I agree it will be a significant effort.

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

I don't think there's a need to drop it. As commented separately, the
code isn't significantly worse, and if this fixes issues and doesn't
cause regressions, it's OK for now as we have other priorities than
rewriting this implementation.

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