[libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update BDS calculation process
Jacopo Mondi
jacopo at jmondi.org
Wed May 12 09:46:25 CEST 2021
Hi Hiro, everyone
On Wed, May 12, 2021 at 09:16:17AM +0200, Jacopo Mondi wrote:
> Hi Hiro,
> Yes sorry
>
> On Wed, May 12, 2021 at 01:23:14PM +0900, Hirokazu Honda wrote:
> > >> > > > + 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
> > >>
> > >>
> > > The variable in the python script is (signed) integer.
> > > So overflow doesn't happen.
> > > The code may work even if width < IF_CROP_MAX_W (I don't check the code
> > > any more than it)?
> > > On the other hand, our code doesn't work because the following while-loop
> > > doesn't loop..?
> > > I see your point. It is the best to set the minimum allowed value after we
> > > get their feedback.
> > > But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround to the
> > > overflow with todo comment?
> > > Well, since the code of causing the overflow is in top-of-tree, I don't
> > > mind merging this.
> > >
> > >
> >
> > Ping. Can you respond to this before you merge this patch?
> >
>
> I've taken your suggestion in
>
> I now have got in my wip v3 version:
>
> + unsigned int minIfWidth = std::min(IF_ALIGN_W,
> + in.width - IF_CROP_MAX_W);
> + unsigned int minIfHeight = std::min(IF_ALIGN_H,
> + in.height - IF_CROP_MAX_H);
>
> I will send it out anyway before merging.
>
Run some more testing. With the above change, the number of
resolutions that are actually tested quickly becomes not manageable at
run time.
Just starting the HAL takes 5 seconds per each camera and the number
of tested configurations goes up to HALF A MILLION (for each
resolutions combination we test at HAL startup time).
The overflow helps skipping a lot of configurations, with a maximum of
30.000 or so. Which is anyway awful but acceptable as the HAL startup
delay is barely noticeable.
I'm not sure how to move forward, if we just skip heights <
IF_CROP_MAX_H (which is 540) to avoid the overflow, we would rule out
sensors that produces images smaller than 540. This happens today
already so it won't technically be a regression, it's just bad.
> >
> > > -Hiro
> > >
> > > 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
> > >> > >
> > >>
> > >
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list