| Message ID | 20210318103941.18837-2-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 the last three hunks of commit 243d134 ("Fix some bug for some > resolutions") from https://github.com/intel/intel-ipu3-pipecfg.git > to the BSD calculation procedure. s/BSD/BDS ? > The BSD calculation is now perfomed by scaling both width and height, > and repeated by scaling width first. s/BSD/BDS ? > 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 | 30 +++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index d5cf05b0c421..4a1b0a9528f2 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -394,19 +394,39 @@ 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 widht 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) { > + 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 {}; >
Hi Jacopo, Thank you for the patch. On Thu, Mar 18, 2021 at 11:39:35AM +0100, 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 BSD calculation procedure. s/BSD/BDS/ and below too. > The BSD calculation is now perfomed by scaling both width and height, > and repeated by scaling width first. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index d5cf05b0c421..4a1b0a9528f2 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -394,19 +394,39 @@ 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 widht and height. */ s/widht/width/ > + 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; > } While this seems to replicate the python code mentioned in the commit message, the logic seems very strange to me. In the first iteration of the outer loop, the inner loop will iterate over all ifHeight values larger than the minimum. The inner loop will exit with ifHeight having an invalid value. Then the second iteration of the outer loop will try the next ifWidth value, but the inner loop will be skipped, as ifHeight will be < minIfHeight already. I'll reiterate my previous concern: if we merely copy the python code without understanding the logic, this will always remain broken. > > + /* 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) { > + 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 {};
Hi Laurent On Tue, Apr 13, 2021 at 02:18:01AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Mar 18, 2021 at 11:39:35AM +0100, 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 BSD calculation procedure. > > > s/BSD/BDS/ > > and below too. > > > The BSD calculation is now perfomed by scaling both width and height, > > and repeated by scaling width first. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++----- > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > index d5cf05b0c421..4a1b0a9528f2 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > @@ -394,19 +394,39 @@ 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 widht and height. */ > > s/widht/width/ > > > + 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; > > } > > While this seems to replicate the python code mentioned in the commit > message, the logic seems very strange to me. In the first iteration of > the outer loop, the inner loop will iterate over all ifHeight values > larger than the minimum. The inner loop will exit with ifHeight having > an invalid value. Then the second iteration of the outer loop will try > the next ifWidth value, but the inner loop will be skipped, as ifHeight > will be < minIfHeight already. > > I'll reiterate my previous concern: if we merely copy the python code > without understanding the logic, this will always remain broken. > I'm sorry but all I can do, without any documentation and with just the python script as information source about the ImgU pipe configuration procedure is to make sure we stay in sync with the most recent script version and that the two behaves the same. This part would have to be rewritten as it is embarassing, but I'm a bit skeptical about distancing ourselves from the tool, as backporting fixes will become very hard. I'm fine dropping this series if there's a plan to re-implement the pipe configuration. > > > > + /* 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) { > > + 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 {}; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Apr 13, 2021 at 09:46:21AM +0200, Jacopo Mondi wrote: > On Tue, Apr 13, 2021 at 02:18:01AM +0300, Laurent Pinchart wrote: > > On Thu, Mar 18, 2021 at 11:39:35AM +0100, 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 BSD calculation procedure. > > > > s/BSD/BDS/ > > > > and below too. > > > > > The BSD calculation is now perfomed by scaling both width and height, > > > and repeated by scaling width first. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++----- > > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > > index d5cf05b0c421..4a1b0a9528f2 100644 > > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > > @@ -394,19 +394,39 @@ 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 widht and height. */ > > > > s/widht/width/ > > > > > + 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; > > > } > > > > While this seems to replicate the python code mentioned in the commit > > message, the logic seems very strange to me. In the first iteration of > > the outer loop, the inner loop will iterate over all ifHeight values > > larger than the minimum. The inner loop will exit with ifHeight having > > an invalid value. Then the second iteration of the outer loop will try > > the next ifWidth value, but the inner loop will be skipped, as ifHeight > > will be < minIfHeight already. > > > > I'll reiterate my previous concern: if we merely copy the python code > > without understanding the logic, this will always remain broken. > > I'm sorry but all I can do, without any documentation and with just > the python script as information source about the ImgU pipe > configuration procedure is to make sure we stay in sync with the most > recent script version and that the two behaves the same. This part > would have to be rewritten as it is embarassing, but I'm a bit > skeptical about distancing ourselves from the tool, as backporting > fixes will become very hard. We have various tools at our disposal, one of them being reverse-engineering :-) I don't agree that there's nothing we can do, but I agree it will be a significant effort. > I'm fine dropping this series if there's a plan to re-implement the > pipe configuration. I don't think there's a need to drop it. As commented separately, the code isn't significantly worse, and if this fixes issues and doesn't cause regressions, it's OK for now as we have other priorities than rewriting this implementation. > > > + /* 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) { > > > + 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 {};
Hi Laurent, On Tue, Apr 13, 2021 at 02:18:01AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Mar 18, 2021 at 11:39:35AM +0100, 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 BSD calculation procedure. > > > s/BSD/BDS/ > > and below too. > > > The BSD calculation is now perfomed by scaling both width and height, > > and repeated by scaling width first. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++----- > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > index d5cf05b0c421..4a1b0a9528f2 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > @@ -394,19 +394,39 @@ 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 widht and height. */ > > s/widht/width/ > > > + 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; > > } > > While this seems to replicate the python code mentioned in the commit > message, the logic seems very strange to me. In the first iteration of > the outer loop, the inner loop will iterate over all ifHeight values > larger than the minimum. The inner loop will exit with ifHeight having > an invalid value. Then the second iteration of the outer loop will try > the next ifWidth value, but the inner loop will be skipped, as ifHeight > will be < minIfHeight already. Indeed, ifHeight should be re-initialized I've opened an issue on the script repository to report this https://github.com/intel/intel-ipu3-pipecfg/issues/2 > > I'll reiterate my previous concern: if we merely copy the python code > without understanding the logic, this will always remain broken. > > > > > + /* 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) { > > + 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 {}; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index d5cf05b0c421..4a1b0a9528f2 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -394,19 +394,39 @@ 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 widht 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) { + 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 BSD calculation procedure. The BSD calculation is now perfomed by scaling both width and height, and repeated by scaling width first. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/imgu.cpp | 30 +++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)