| Message ID | 20210318103941.18837-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for the patch, On 18/03/2021 11:39, Jacopo Mondi wrote: > Apply to calculateBDSHeight() function the first hunk of commit 243d134 > ("Fix some bug for some resolutions") from > https://github.com/intel/intel-ipu3-pipecfg.git. > > The condition for the computed IF rectangle height to be matched > against the desired alignment now makes sure that it is included > in the minimum and maximum acceptable values. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index 11bb7cb95084..89a01bddbed2 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height); > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > - while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && > + ifHeight / bdsSF >= minBDSHeight) { > > bdsHeight = ifHeight / bdsSF; > if (std::fmod(bdsHeight, 1.0) == 0) { > @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > } > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > - while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && > + ifHeight / bdsSF >= minBDSHeight) { > > bdsHeight = ifHeight / bdsSF; > if (std::fmod(bdsHeight, 1.0) == 0) { >
Hi Jacopo, Thank you for the patch. On Thu, Mar 18, 2021 at 11:39:38AM +0100, Jacopo Mondi wrote: > Apply to calculateBDSHeight() function the first hunk of commit 243d134 > ("Fix some bug for some resolutions") from > https://github.com/intel/intel-ipu3-pipecfg.git. > > The condition for the computed IF rectangle height to be matched > against the desired alignment now makes sure that it is included > in the minimum and maximum acceptable values. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index 11bb7cb95084..89a01bddbed2 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height); > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > - while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && iif.height isn't changed in the loop, and ifHeight is only decreased. The new condition can either be false on the first iteration, in which case the loop will be skipped completely, or it will always be true and won't have an effect. > + ifHeight / bdsSF >= minBDSHeight) { > > bdsHeight = ifHeight / bdsSF; > if (std::fmod(bdsHeight, 1.0) == 0) { > @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > } > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > - while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && If the condition was false on the first iteration of the previous loop, if will still be false here, and both loops will be skipped. There's something fishy in this code. It doesn't seem significantly worse after this change though. > + ifHeight / bdsSF >= minBDSHeight) { > > bdsHeight = ifHeight / bdsSF; > if (std::fmod(bdsHeight, 1.0) == 0) {
Hi Laurent, On Tue, Apr 13, 2021 at 03:14:43AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Mar 18, 2021 at 11:39:38AM +0100, Jacopo Mondi wrote: > > Apply to calculateBDSHeight() function the first hunk of commit 243d134 > > ("Fix some bug for some resolutions") from > > https://github.com/intel/intel-ipu3-pipecfg.git. > > > > The condition for the computed IF rectangle height to be matched > > against the desired alignment now makes sure that it is included > > in the minimum and maximum acceptable values. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > index 11bb7cb95084..89a01bddbed2 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height); > > > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > > - while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > iif.height isn't changed in the loop, and ifHeight is only decreased. > The new condition can either be false on the first iteration, in which > case the loop will be skipped completely, or it will always be true and > won't have an effect. > > > + ifHeight / bdsSF >= minBDSHeight) { > > > > bdsHeight = ifHeight / bdsSF; > > if (std::fmod(bdsHeight, 1.0) == 0) { > > @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > } > > > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > > - while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { > > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > If the condition was false on the first iteration of the previous loop, > if will still be false here, and both loops will be skipped. There's > something fishy in this code. It doesn't seem significantly worse after > this change though. > Correct, but in this second loop ifHeight is increased, so the check is required. I agree in the first loop it could have been a condition external to the while() but it would be one indentation level that can be skipped imho. Again, deviating from the script is risky in terms of eventual backporting > > + ifHeight / bdsSF >= minBDSHeight) { > > > > bdsHeight = ifHeight / bdsSF; > > if (std::fmod(bdsHeight, 1.0) == 0) { > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Wed, Apr 21, 2021 at 03:01:18PM +0200, Jacopo Mondi wrote: > On Tue, Apr 13, 2021 at 03:14:43AM +0300, Laurent Pinchart wrote: > > On Thu, Mar 18, 2021 at 11:39:38AM +0100, Jacopo Mondi wrote: > > > Apply to calculateBDSHeight() function the first hunk of commit 243d134 > > > ("Fix some bug for some resolutions") from > > > https://github.com/intel/intel-ipu3-pipecfg.git. > > > > > > The condition for the computed IF rectangle height to be matched > > > against the desired alignment now makes sure that it is included > > > in the minimum and maximum acceptable values. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > > index 11bb7cb95084..89a01bddbed2 100644 > > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > > @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > > estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height); > > > > > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > > > - while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > > > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > > > iif.height isn't changed in the loop, and ifHeight is only decreased. > > The new condition can either be false on the first iteration, in which > > case the loop will be skipped completely, or it will always be true and > > won't have an effect. > > > > > + ifHeight / bdsSF >= minBDSHeight) { > > > > > > bdsHeight = ifHeight / bdsSF; > > > if (std::fmod(bdsHeight, 1.0) == 0) { > > > @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > > } > > > > > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > > > - while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { > > > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > > > If the condition was false on the first iteration of the previous loop, > > if will still be false here, and both loops will be skipped. There's > > something fishy in this code. It doesn't seem significantly worse after > > this change though. > > > > Correct, but in this second loop ifHeight is increased, so the check > is required. I agree in the first loop it could have been a condition > external to the while() but it would be one indentation level that can > be skipped imho. > > Again, deviating from the script is risky in terms of eventual backporting As commented on patch 3/7, I don't think that's a big issue, we shouldn't duplicate the python code in the first place. > > > + ifHeight / bdsSF >= minBDSHeight) { > > > > > > bdsHeight = ifHeight / bdsSF; > > > if (std::fmod(bdsHeight, 1.0) == 0) {
Hi Laurent On Tue, Apr 13, 2021 at 03:14:43AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Mar 18, 2021 at 11:39:38AM +0100, Jacopo Mondi wrote: > > Apply to calculateBDSHeight() function the first hunk of commit 243d134 > > ("Fix some bug for some resolutions") from > > https://github.com/intel/intel-ipu3-pipecfg.git. > > > > The condition for the computed IF rectangle height to be matched > > against the desired alignment now makes sure that it is included > > in the minimum and maximum acceptable values. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > index 11bb7cb95084..89a01bddbed2 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height); > > > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > > - while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > iif.height isn't changed in the loop, and ifHeight is only decreased. > The new condition can either be false on the first iteration, in which > case the loop will be skipped completely, or it will always be true and > won't have an effect. > > > + ifHeight / bdsSF >= minBDSHeight) { > > > > bdsHeight = ifHeight / bdsSF; > > if (std::fmod(bdsHeight, 1.0) == 0) { > > @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > } > > > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > > - while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { > > + while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > If the condition was false on the first iteration of the previous loop, > if will still be false here, and both loops will be skipped. There's > something fishy in this code. It doesn't seem significantly worse after > this change though. You know, I presume this might even be intended, even if it was probably better coded as if (ifHeight > iif.height) { while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { .... ifHeight -= IF_ALIGN_H; } while (ifHeight >= minIFHeight && ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { .... ifHeight += IF_ALIGN_H; } } > > > + ifHeight / bdsSF >= minBDSHeight) { > > > > bdsHeight = ifHeight / bdsSF; > > if (std::fmod(bdsHeight, 1.0) == 0) { > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index 11bb7cb95084..89a01bddbed2 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -135,7 +135,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height); ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); - while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { + while (ifHeight >= minIFHeight && ifHeight <= iif.height && + ifHeight / bdsSF >= minBDSHeight) { bdsHeight = ifHeight / bdsSF; if (std::fmod(bdsHeight, 1.0) == 0) { @@ -151,7 +152,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc } ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); - while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { + while (ifHeight >= minIFHeight && ifHeight <= iif.height && + ifHeight / bdsSF >= minBDSHeight) { bdsHeight = ifHeight / bdsSF; if (std::fmod(bdsHeight, 1.0) == 0) {
Apply to calculateBDSHeight() function the first hunk of commit 243d134 ("Fix some bug for some resolutions") from https://github.com/intel/intel-ipu3-pipecfg.git. The condition for the computed IF rectangle height to be matched against the desired alignment now makes sure that it is included in the minimum and maximum acceptable values. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/imgu.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)