[libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update BDS calculation process
Hirokazu Honda
hiroh at chromium.org
Thu May 13 04:19:19 CEST 2021
Hi Jacopo,
On Wed, May 12, 2021 at 5:45 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Hiro,
>
> On Wed, May 12, 2021 at 04:56:19PM +0900, Hirokazu Honda wrote:
> > Hi Jacopo,
> >
> > On Wed, May 12, 2021 at 4:45 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > > 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.
> > >
> > >
> > Wow... it is pretty bad.
>
> yes indeed.
>
> > IMHO, we can go ahead temporarily with the overflow leaving a todo
> comment
> > and let's file a bug about it as this is not a regression introduced by
> > this.
> > We may want to discuss in the issue tracker.
>
> I've not recorded it in a bug, and will refer to that in the code
> https://bugs.libcamera.org/show_bug.cgi?id=32
>
> For the moment I feel it would be safer to skip all input resolutions <
> IF_CROP_MAX
>
>
Ack. Thanks!
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
-Hiro
> Thanks
> j
>
> >
> > -Hiro
> >
> > > >
> > > > > > -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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210513/6fd8b329/attachment-0001.htm>
More information about the libcamera-devel
mailing list