<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 6, 2021 at 5:15 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Thu, May 06, 2021 at 02:34:30PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo, thank you for the patch.<br>
><br>
> On Mon, May 3, 2021 at 7:56 PM Niklas Söderlund <<br>
> <a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>> wrote:<br>
><br>
> > Hi Jacopo,<br>
> ><br>
> > Thanks for your work.<br>
> ><br>
> > On 2021-05-03 11:26:59 +0200, Jacopo Mondi wrote:<br>
> > > Apply the last three hunks of commit 243d134 ("Fix some bug for some<br>
> > > resolutions") from <a href="https://github.com/intel/intel-ipu3-pipecfg.git" rel="noreferrer" target="_blank">https://github.com/intel/intel-ipu3-pipecfg.git</a><br>
> > > to the BDS calculation procedure.<br>
> > ><br>
> > > The BDS calculation is now perfomed by scaling both width and height,<br>
> > > and repeated by scaling width first.<br>
> > ><br>
> > > Tested-by: Jean-Michel Hautbois <<a href="mailto:jeanmichel.hautbois@ideasonboard.com" target="_blank">jeanmichel.hautbois@ideasonboard.com</a>><br>
> > > Reviewed-by: Jean-Michel Hautbois <<a href="mailto:jeanmichel.hautbois@ideasonboard.com" target="_blank">jeanmichel.hautbois@ideasonboard.com</a>><br>
> > > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> ><br>
> > Acked-by: Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>><br>
> ><br>
> > > ---<br>
> > > src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++++++++++----<br>
> > > 1 file changed, 29 insertions(+), 5 deletions(-)<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp<br>
> > b/src/libcamera/pipeline/ipu3/imgu.cpp<br>
> > > index d5cf05b0c421..8373dc165a61 100644<br>
> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp<br>
> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp<br>
> > > @@ -394,19 +394,43 @@ ImgUDevice::PipeConfig<br>
> > ImgUDevice::calculatePipeConfig(Pipe *pipe)<br>
> > > const Size &in = pipe->input;<br>
> > > Size gdc = calculateGDC(pipe);<br>
> > ><br>
> > > - unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);<br>
> > > - unsigned int ifHeight = in.height;<br>
> > > - unsigned int minIfWidth = in.width - IF_CROP_MAX_W;<br>
> > > float bdsSF = static_cast<float>(in.width) / gdc.width;<br>
> > > float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);<br>
> > ><br>
> > > + /* Populate the configurations vector by scaling width and height.<br>
> > */<br>
> > > + unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);<br>
> > > + unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);<br>
> > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;<br>
> > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;<br>
> ><br>
><br>
> Should we std::max(0u, in.width - IF_CROP_MAX_W)?<br>
<br>
I would be careful and not use 0, but probably 2 and 4 as it seems to be the<br>
vertical and horizontal alignments the ImgU requires<br>
<br>
> If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes<br>
> very large.<br>
> Ditto to height.<br>
<br>
Laurent had the same question, and we received patch in the past that<br>
was probably related to an issue like this one, if the input size is<br>
smaller than 540 (as IF_CROP_MAX_H=540).<br>
<a href="https://patchwork.libcamera.org/patch/11620/" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/11620/</a><br>
<br>
I've opened (yet) a new issue on the python script this code is based<br>
on to have Intel's opinion on which is the min IF crop size<br>
<a href="https://github.com/intel/intel-ipu3-pipecfg/issues/3" rel="noreferrer" target="_blank">https://github.com/intel/intel-ipu3-pipecfg/issues/3</a><br>
<br></blockquote><div><br></div><div>The variable in the python script is (signed) integer.</div><div>So overflow doesn't happen.</div><div>The code may work even if width < IF_CROP_MAX_W (I don't check the code any more than it)?</div><div>On the other hand, our code doesn't work because the following while-loop doesn't loop..?</div><div>I see your point. It is the best to set the minimum allowed value after we get their feedback.</div><div>But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround to the overflow with todo comment?</div><div>Well, since the code of causing the overflow is in top-of-tree, I don't mind merging this.</div><div> </div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I'm thinking of fixing this issue, and the other ones reported during<br>
review of this series (and reflected as issues on the script's github)<br>
on top.<br>
<br>
I'm overly-concerned with the idea of deviating from the script. I've<br>
received several comments on how things could have been done better,<br>
but I'm pushing back as I want this code to be as similar as possible<br>
to the intel's script, otherwise every 4 months when (I have to)<br>
compare the two implementation my brain will melt even more than<br>
usual.<br>
<br>
Result: the code is of awful quality as the script is.<br>
<br>
I re-state the requirement from Intel to provide documentation on<br>
how the ImgU sizes have to be computed instead of a buggy script<br>
(which I apreciate a lot don't get me wrong, I'm sure the author has<br>
put some of his free time to implement that as I'm quite sure nobody<br>
in there apreciate spending more company time on IPU3, but the result<br>
is a best effort implementation nobody is proud of).<br>
<br>
Even better if they could do directly in libcamera... a man can<br>
dream... So far we received close to 0 attention from them and surely<br>
we have no leverage from here to convince them of the contrary :)<br>
Maybe you could push a little to have them consider us a little<br>
bit more ?<br>
<br>
Thanks<br>
j<br>
<br>
><br>
><br>
> > > while (ifWidth >= minIfWidth) {<br>
> > > - Size iif{ ifWidth, ifHeight };<br>
> > > - calculateBDS(pipe, iif, gdc, sf);<br>
> > > + while (ifHeight >= minIfHeight) {<br>
> > > + Size iif{ ifWidth, ifHeight };<br>
> > > + calculateBDS(pipe, iif, gdc, sf);<br>
> > > + ifHeight -= IF_ALIGN_H;<br>
> > > + }<br>
> > ><br>
> > > ifWidth -= IF_ALIGN_W;<br>
> > > }<br>
> > ><br>
> > > + /* Repeat search by scaling width first. */<br>
> > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);<br>
> > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);<br>
> > > + minIfWidth = in.width - IF_CROP_MAX_W;<br>
> > > + minIfHeight = in.height - IF_CROP_MAX_H;<br>
> > > + while (ifHeight >= minIfHeight) {<br>
> > > + /*<br>
> > > + * \todo This procedure is probably broken:<br>
> > > + * <a href="https://github.com/intel/intel-ipu3-pipecfg/issues/2" rel="noreferrer" target="_blank">https://github.com/intel/intel-ipu3-pipecfg/issues/2</a><br>
> > > + */<br>
> > > + while (ifWidth >= minIfWidth) {<br>
> > > + Size iif{ ifWidth, ifHeight };<br>
> > > + calculateBDS(pipe, iif, gdc, sf);<br>
> > > + ifWidth -= IF_ALIGN_W;<br>
> > > + }<br>
> > > +<br>
> > > + ifHeight -= IF_ALIGN_H;<br>
> > > + }<br>
> > > +<br>
> > > if (pipeConfigs.size() == 0) {<br>
> > > LOG(IPU3, Error) << "Failed to calculate pipe<br>
> > configuration";<br>
> > > return {};<br>
> > > --<br>
> > > 2.31.1<br>
> > ><br>
> > > _______________________________________________<br>
> > > libcamera-devel mailing list<br>
> > > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
> > --<br>
> > Regards,<br>
> > Niklas Söderlund<br>
> > _______________________________________________<br>
> > libcamera-devel mailing list<br>
> > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
</blockquote></div></div>