| Message ID | 20210503092705.15562-2-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Acked-by: Niklas Söderlund <niklas.soderlund@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; > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, thank you for the patch. On Mon, May 3, 2021 at 7:56 PM Niklas Söderlund < niklas.soderlund@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@ideasonboard.com> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Acked-by: Niklas Söderlund <niklas.soderlund@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)? If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes very large. Ditto to height. > > 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
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@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@ideasonboard.com> > > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Acked-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Jacopo, On Thu, May 6, 2021 at 5:15 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > 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@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@ideasonboard.com> > > > > Reviewed-by: Jean-Michel Hautbois < > jeanmichel.hautbois@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Acked-by: Niklas Söderlund <niklas.soderlund@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 > > 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. -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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >
On Fri, May 7, 2021 at 12:13 PM Hirokazu Honda <hiroh@chromium.org> wrote: > Hi Jacopo, > > On Thu, May 6, 2021 at 5:15 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > >> 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@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@ideasonboard.com> >> > > > Reviewed-by: Jean-Michel Hautbois < >> jeanmichel.hautbois@ideasonboard.com> >> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >> > > >> > > Acked-by: Niklas Söderlund <niklas.soderlund@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 >> >> > 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? > -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@lists.libcamera.org >> > > > https://lists.libcamera.org/listinfo/libcamera-devel >> > > >> > > -- >> > > Regards, >> > > Niklas Söderlund >> > > _______________________________________________ >> > > libcamera-devel mailing list >> > > libcamera-devel@lists.libcamera.org >> > > https://lists.libcamera.org/listinfo/libcamera-devel >> > > >> >
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. > > > -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@lists.libcamera.org > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel > >> > > > >> > > -- > >> > > Regards, > >> > > Niklas Söderlund > >> > > _______________________________________________ > >> > > libcamera-devel mailing list > >> > > libcamera-devel@lists.libcamera.org > >> > > https://lists.libcamera.org/listinfo/libcamera-devel > >> > > > >> > >
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@lists.libcamera.org > > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel > > >> > > > > >> > > -- > > >> > > Regards, > > >> > > Niklas Söderlund > > >> > > _______________________________________________ > > >> > > libcamera-devel mailing list > > >> > > libcamera-devel@lists.libcamera.org > > >> > > https://lists.libcamera.org/listinfo/libcamera-devel > > >> > > > > >> > > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Wed, May 12, 2021 at 4:45 PM Jacopo Mondi <jacopo@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. 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. -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@lists.libcamera.org > > > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >> > > > > > >> > > -- > > > >> > > Regards, > > > >> > > Niklas Söderlund > > > >> > > _______________________________________________ > > > >> > > libcamera-devel mailing list > > > >> > > libcamera-devel@lists.libcamera.org > > > >> > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >> > > > > > >> > > > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
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@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 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@lists.libcamera.org > > > > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > >> > > > > > > >> > > -- > > > > >> > > Regards, > > > > >> > > Niklas Söderlund > > > > >> > > _______________________________________________ > > > > >> > > libcamera-devel mailing list > > > > >> > > libcamera-devel@lists.libcamera.org > > > > >> > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > >> > > > > > > >> > > > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Jacopo, On Wed, May 12, 2021 at 5:45 PM Jacopo Mondi <jacopo@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@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@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@lists.libcamera.org > > > > > >> > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > >> > > > > > > > >> > > -- > > > > > >> > > Regards, > > > > > >> > > Niklas Söderlund > > > > > >> > > _______________________________________________ > > > > > >> > > libcamera-devel mailing list > > > > > >> > > libcamera-devel@lists.libcamera.org > > > > > >> > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > >> > > > > > > > >> > > > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >
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; 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 {};
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@ideasonboard.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-)