[libcamera-devel] [PATCH 3/7] libcamera: ipu3: imgu: Fix BDS height calculation

[Jacopo Mondi]

Hi Laurent,

On Tue, Apr 13, 2021 at 03:18:02AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Mar 18, 2021 at 11:39:37AM +0100, Jacopo Mondi wrote:
> > The IF rectangle height is iteratively computed by first subtracting
> > the scaling factor to the estimated height, then in a successive loop
> > by adding the same scaling factor until the maximum IF size is not
> > reached.
> >
> > As the computed IF height is not cached in any variable, the second
> > loop over-writes the result of the first one, even if the BDS alignment
> > condition is not satisfied.
> >
> > Fix this by caching the result of the two iterations, and use the one
> > that produced any result, with a preference for the one produced by the
> > second loop, as implemented in the reference python script.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index d9296ea3e674..11bb7cb95084 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -129,10 +129,10 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> >  	float bdsHeight;
> >
> >  	if (!isSameRatio(pipe->input, gdc)) {
> > +		unsigned int foundIfHeights[2] = { 0, 0 };
>
> You don't need two values, a single one will do, it can be overwritten
> in the second loop.

Correct, I got the same understanding but I would prefer to use the
same "style" as the python script, not to diverge...

What do you think ?

>
> With an updated commit message,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks
  j

>
> >  		float estIFHeight = (iif.width * gdc.height) /
> >  				    static_cast<float>(gdc.width);
> >  		estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);
> > -		bool found = false;
> >
> >  		ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> >  		while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {
> > @@ -142,7 +142,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> >  				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> >
> >  				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > -					found = true;
> > +					foundIfHeights[0] = ifHeight;
> >  					break;
> >  				}
> >  			}
> > @@ -158,7 +158,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> >  				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> >
> >  				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > -					found = true;
> > +					foundIfHeights[1] = ifHeight;
> >  					break;
> >  				}
> >  			}
> > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> >  			ifHeight += IF_ALIGN_H;
> >  		}
> >
> > -		if (found) {
> > +		if (foundIfHeights[0])
> > +			ifHeight = foundIfHeights[0];
> > +		if (foundIfHeights[1])
> > +			ifHeight = foundIfHeights[1];
> > +
> > +		if (foundIfHeights[0] || foundIfHeights[1]) {
> >  			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> >
> >  			pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
>
> --
> Regards,
>
> Laurent Pinchart