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

[Jacopo Mondi]

Hi Hiro,

On Thu, May 06, 2021 at 02:34:30PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for the patch.
>
> On Mon, May 3, 2021 at 7:56 PM Niklas Söderlund <
> niklas.soderlund at ragnatech.se> wrote:
>
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2021-05-03 11:26:59 +0200, 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 BDS calculation procedure.
> > >
> > > The BDS calculation is now perfomed by scaling both width and height,
> > > and repeated by scaling width first.
> > >
> > > Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > Acked-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >
> > > ---
> > >  src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++++++++++----
> > >  1 file changed, 29 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp
> > b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > index d5cf05b0c421..8373dc165a61 100644
> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > @@ -394,19 +394,43 @@ 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 width and height.
> > */
> > > +     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;
> >
>
> Should we std::max(0u, in.width - IF_CROP_MAX_W)?

I would be careful and not use 0, but probably 2 and 4 as it seems to be the
vertical and horizontal alignments the ImgU requires

> If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes
> very large.
> Ditto to height.

Laurent had the same question, and we received patch in the past that
was probably related to an issue like this one, if the input size is
smaller than 540 (as IF_CROP_MAX_H=540).
https://patchwork.libcamera.org/patch/11620/

I've opened (yet) a new issue on the python script this code is based
on to have Intel's opinion on which is the min IF crop size
https://github.com/intel/intel-ipu3-pipecfg/issues/3

I'm thinking of fixing this issue, and the other ones reported during
review of this series (and reflected as issues on the script's github)
on top.

I'm overly-concerned with the idea of deviating from the script. I've
received several comments on how things could have been done better,
but I'm pushing back as I want this code to be as similar as possible
to the intel's script, otherwise every 4 months when (I have to)
compare the two implementation my brain will melt even more than
usual.

Result: the code is of awful quality as the script is.

I re-state the requirement from Intel to provide documentation on
how the ImgU sizes have to be computed instead of a buggy script
(which I apreciate a lot don't get me wrong, I'm sure the author has
put some of his free time to implement that as I'm quite sure nobody
in there apreciate spending more company time on IPU3, but the result
is a best effort implementation nobody is proud of).

Even better if they could do directly in libcamera... a man can
dream... So far we received close to 0 attention from them and surely
we have no leverage from here to convince them of the contrary :)
Maybe you could push a little to have them consider us a little
bit more ?

Thanks
   j

>
>
> > >       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;
> > >       }
> > >
> > > +     /* 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) {
> > > +             /*
> > > +              * \todo This procedure is probably broken:
> > > +              * https://github.com/intel/intel-ipu3-pipecfg/issues/2
> > > +              */
> > > +             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 {};
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >