| Message ID | 20200731153320.58107-1-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
Hi Jacopo, Thank you for the patches. On Fri, Jul 31, 2020 at 05:33:01PM +0200, Jacopo Mondi wrote: > I have addressed minor comments and added a few more details in comment and > documentation here and there. > > I have simplified a bit the ImgU pipe configuration by removing not-required > casts, by making the code a bit less dense, and with a small rename. > > The diff between v4 and v5 is so minimal I reported the full diff below. I'll thus review it below :-) > I tested results against the IPU3 pipe configuration tool by instrumenting > a version of it to run on a set of known values and compare the results between > its calculation and libcamera: > https://jmondi.org/cgit/intel-ipu3-pipe-config/commit/?id=78bc59f8bfe8c14908f612008a62edc621660cf3 > > All patches reviewed but I'm not pushing yet as I would like Kieran's ack, as > he's working on JPEG and moving the ground under his feet while developing seems > not nice. > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index a3ea3eabe318..cf26980c75f1 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -25,6 +25,14 @@ LOG_DECLARE_CATEGORY(IPU3) > > namespace { > > +/* > + * The procedure to calculate the ImgU pipe configuration has been ported > + * from the pipe_config.py python script, available at: > + * https://github.com/intel/intel-ipu3-pipecfg > + * at revision: 61e83f2f7606 ("Add more information into README") > + */ > + > static constexpr unsigned int FILTER_H = 4; > > static constexpr unsigned int IF_ALIGN_W = 2; > @@ -103,10 +111,8 @@ float findScaleFactor(float sf, const std::vector<float> &range, > > bool isSameRatio(const Size &in, const Size &out) > { > - float inRatio = static_cast<float>(in.width) / > - static_cast<float>(in.height); > - float outRatio = static_cast<float>(out.width) / > - static_cast<float>(out.height); > + float inRatio = static_cast<float>(in.width) / in.height; > + float outRatio = static_cast<float>(out.width) / out.height; > > if (std::abs(inRatio - outRatio) > 0.1) > return false; > @@ -123,16 +129,18 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > float bdsHeight; > > if (!isSameRatio(pipe->input, gdc)) { > - bool found = false; > - float estIFHeight = static_cast<float>(iif.width * gdc.height) / > + float estIFHeight = (iif.width * gdc.height) / > static_cast<float>(gdc.width); > estIFHeight = utils::clamp<float>(estIFHeight, minIFHeight, iif.height); > + bool found = false; > + > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > - while (ifHeight >= minIFHeight && > - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { > - bdsHeight = static_cast<float>(ifHeight) / bdsSF; > + while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > + > + bdsHeight = ifHeight / bdsSF; > if (std::fmod(bdsHeight, 1.0) == 0) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > + > if (!(bdsIntHeight % BDS_ALIGN_H)) { > found = true; > break; > @@ -143,11 +151,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > } > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > - while (ifHeight <= iif.height && > - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { > - bdsHeight = static_cast<float>(ifHeight) / bdsSF; > + while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { > + > + bdsHeight = ifHeight / bdsSF; > if (std::fmod(bdsHeight, 1.0) == 0) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > + > if (!(bdsIntHeight % BDS_ALIGN_H)) { > found = true; > break; > @@ -159,14 +168,15 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > if (found) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > + > pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight }, > { bdsWidth, bdsIntHeight }, gdc }); > return; > } > } else { > ifHeight = utils::alignUp(iif.height, IF_ALIGN_H); > - while (ifHeight > minIFHeight && > - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { > + while (ifHeight > minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > + > bdsHeight = ifHeight / bdsSF; > if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > @@ -183,8 +193,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > } > } > > -void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, > - float bdsSF) > +void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, float bdsSF) > { > unsigned int minBDSWidth = gdc.width + FILTER_H * 2; > > @@ -218,15 +227,14 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, > Size calculateGDC(ImgUDevice::Pipe *pipe) > { > const Size &in = pipe->input; > - const Size &main = pipe->output; > + const Size &main = pipe->main; > const Size &vf = pipe->viewfinder; > Size gdc; > > if (!vf.isNull()) { > gdc.width = main.width; > > - float ratio = static_cast<float>(main.width * vf.height) / > - static_cast<float>(vf.width); > + float ratio = (main.width * vf.height) / static_cast<float>(vf.width); > gdc.height = std::max(static_cast<float>(main.height), ratio); > > return gdc; > @@ -237,14 +245,13 @@ Size calculateGDC(ImgUDevice::Pipe *pipe) > return gdc; > } > > - float totalSF = static_cast<float>(in.width) / > - static_cast<float>(main.width); > + float totalSF = static_cast<float>(in.width) / main.width; > float bdsSF = totalSF > 2 ? 2 : 1; > float yuvSF = totalSF / bdsSF; > float sf = findScaleFactor(yuvSF, gdcScalingFactors); > > - gdc.width = static_cast<float>(main.width) * sf; > - gdc.height = static_cast<float>(main.height) * sf; > + gdc.width = main.width * sf; > + gdc.height = main.height * sf; > > return gdc; > } > @@ -259,6 +266,7 @@ FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe) > float ifCropH = static_cast<float>(in.height - pipe.iif.height); > float gdcCropW = static_cast<float>(pipe.bds.width - pipe.gdc.width) * pipe.bds_sf; > float gdcCropH = static_cast<float>(pipe.bds.height - pipe.gdc.height) * pipe.bds_sf; > + > fov.w = (inW - (ifCropW + gdcCropW)) / inW; > fov.h = (inH - (ifCropH + gdcCropH)) / inH; > > @@ -304,11 +312,11 @@ FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe) > * \var Pipe::input > * \brief The input image size > * > - * \var Pipe::output > + * \var Pipe::main > * \brief The requested main output size > * > * \var Pipe::viewfinder > - * \brief The requested viewfinder size > + * \brief The requested viewfinder output size > */ > > /** > @@ -378,7 +386,7 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe) > > LOG(IPU3, Debug) << "Calculating pipe configuration for: "; > LOG(IPU3, Debug) << "input: " << pipe->input.toString(); > - LOG(IPU3, Debug) << "main: " << pipe->output.toString(); > + LOG(IPU3, Debug) << "main: " << pipe->main.toString(); > LOG(IPU3, Debug) << "vf: " << pipe->viewfinder.toString(); > > const Size &in = pipe->input; > @@ -387,9 +395,9 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *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) / > - static_cast<float>(gdc.width); > + float bdsSF = static_cast<float>(in.width) / gdc.width; > float sf = findScaleFactor(bdsSF, bdsScalingFactors, true); > + > while (ifWidth >= minIfWidth) { > Size iif{ ifWidth, ifHeight }; > calculateBDS(pipe, iif, gdc, sf); > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > index ff8dab7d645c..c73ac5a5a37c 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.h > +++ b/src/libcamera/pipeline/ipu3/imgu.h > @@ -37,7 +37,7 @@ public: > > struct Pipe { > Size input; > - Size output; > + Size main; > Size viewfinder; > }; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 06afc7d79dac..161a88da8497 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -225,11 +225,12 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > * margins from the CIO2 output frame size. > * > * The ImgU outputs needs to be strictly smaller than > - * the CIO2 output frame and rounded down to 64 > - * pixels in width and 32 pixels in height. This comes > - * from inspecting the pipe configuration script results > - * and the available suggested configurations in the > - * ChromeOS BSP .xml camera tuning files. > + * the CIO2 output frame and rounded down to 64 pixels > + * in width and 32 pixels in height. This assumption > + * comes from inspecting the pipe configuration script > + * results and the available suggested configurations in > + * the ChromeOS BSP .xml camera tuning files and shall > + * be validated. > * > * \todo Clarify what are the hardware constraints > * that require this alignements, if any. It might > @@ -267,9 +268,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > cfg->setStream(const_cast<Stream *>(&data_->outStream_)); > mainOutputAvailable = false; > > - pipe.output = cfg->size; > + pipe.main = cfg->size; > if (yuvCount == 1) > - pipe.viewfinder = pipe.output; > + pipe.viewfinder = pipe.main; > > LOG(IPU3, Debug) << "Assigned " << cfg->toString() > << " to the main output"; > @@ -333,8 +334,8 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > * to the ImgU maximum output size) and aligned down to > * the required frame margin. > * > - * The alignment constraints should be clarified > - * as per the todo item in validate(). > + * \todo Clarify the alignment constraints as exaplained s/exaplained/explained/ > + * in validate() > */ > size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE); > size.width = utils::alignDown(size.width - 1, > > Jacopo Mondi (19): > libcamera: ipu3: Rename mbusCodesToInfo > libcamera: utils: Add alignUp and alignDown functions > libcamera: geometry: Add isNull() function to Rectangle class > libcamera: ipu3: Remove streams from generateConfiguration > libcamera: ipu3: Make sure the config is valid > libcamera: ipu3: cio2: Report format and sizes > libcamera: ipu3: Do not overwrite StreamConfiguration > libcamera: ipu3: Report StreamFormats > libcamera: ipu3: Remove initialization of Size > libcamera: ipu3: Validate the stream combination > libcamera: camera: Zero streams before validate() > libcamera: ipu3: cio2: Add a const sensor() method > libcamera: ipu3: Adjust and assign streams in validate() > libcamera: ipu3: Remove streams from IPU3CameraConfiguration > libcamera: ipu3: Remove camera_ from IPU3CameraConfiguration > libcamera: ipu3: imgu: Calculate ImgU pipe configuration > libcamera: ipu3: Validate the pipe configuration > libcamera: ipu3: Configure ImgU with the computed parameters > libcamera: ipu3: imgu: Rename configureInput() > > include/libcamera/geometry.h | 1 + > include/libcamera/internal/utils.h | 10 + > src/libcamera/camera.cpp | 4 +- > src/libcamera/geometry.cpp | 6 + > src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++- > src/libcamera/pipeline/ipu3/cio2.h | 6 + > src/libcamera/pipeline/ipu3/imgu. | 0 > src/libcamera/pipeline/ipu3/imgu.cpp | 386 +++++++++++++++++++++++- > src/libcamera/pipeline/ipu3/imgu.h | 22 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 424 +++++++++++++-------------- > src/libcamera/utils.cpp | 16 + > test/geometry.cpp | 14 + > test/utils.cpp | 11 + > 13 files changed, 714 insertions(+), 240 deletions(-) > create mode 100644 src/libcamera/pipeline/ipu3/imgu. >
Hi Jacopo, I've gone through this series quickly, and my work is based upon it. You already seem to have two RB tags for each patch, so rather than do a detailed review, I'll just say "Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>" for the whole series, and I'm happy to get this in and keep pushing forwards :-) -- Kieran On 31/07/2020 16:33, Jacopo Mondi wrote: > I have addressed minor comments and added a few more details in comment and > documentation here and there. > > I have simplified a bit the ImgU pipe configuration by removing not-required > casts, by making the code a bit less dense, and with a small rename. > > The diff between v4 and v5 is so minimal I reported the full diff below. > > I tested results against the IPU3 pipe configuration tool by instrumenting > a version of it to run on a set of known values and compare the results between > its calculation and libcamera: > https://jmondi.org/cgit/intel-ipu3-pipe-config/commit/?id=78bc59f8bfe8c14908f612008a62edc621660cf3 > > All patches reviewed but I'm not pushing yet as I would like Kieran's ack, as > he's working on JPEG and moving the ground under his feet while developing seems > not nice. > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index a3ea3eabe318..cf26980c75f1 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -25,6 +25,14 @@ LOG_DECLARE_CATEGORY(IPU3) > > namespace { > > +/* > + * The procedure to calculate the ImgU pipe configuration has been ported > + * from the pipe_config.py python script, available at: > + * https://github.com/intel/intel-ipu3-pipecfg > + * at revision: 61e83f2f7606 ("Add more information into README") > + */ > + > static constexpr unsigned int FILTER_H = 4; > > static constexpr unsigned int IF_ALIGN_W = 2; > @@ -103,10 +111,8 @@ float findScaleFactor(float sf, const std::vector<float> &range, > > bool isSameRatio(const Size &in, const Size &out) > { > - float inRatio = static_cast<float>(in.width) / > - static_cast<float>(in.height); > - float outRatio = static_cast<float>(out.width) / > - static_cast<float>(out.height); > + float inRatio = static_cast<float>(in.width) / in.height; > + float outRatio = static_cast<float>(out.width) / out.height; > > if (std::abs(inRatio - outRatio) > 0.1) > return false; > @@ -123,16 +129,18 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > float bdsHeight; > > if (!isSameRatio(pipe->input, gdc)) { > - bool found = false; > - float estIFHeight = static_cast<float>(iif.width * gdc.height) / > + float estIFHeight = (iif.width * gdc.height) / > static_cast<float>(gdc.width); > estIFHeight = utils::clamp<float>(estIFHeight, minIFHeight, iif.height); > + bool found = false; > + > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > - while (ifHeight >= minIFHeight && > - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { > - bdsHeight = static_cast<float>(ifHeight) / bdsSF; > + while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > + > + bdsHeight = ifHeight / bdsSF; > if (std::fmod(bdsHeight, 1.0) == 0) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > + > if (!(bdsIntHeight % BDS_ALIGN_H)) { > found = true; > break; > @@ -143,11 +151,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > } > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); > - while (ifHeight <= iif.height && > - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { > - bdsHeight = static_cast<float>(ifHeight) / bdsSF; > + while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { > + > + bdsHeight = ifHeight / bdsSF; > if (std::fmod(bdsHeight, 1.0) == 0) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > + > if (!(bdsIntHeight % BDS_ALIGN_H)) { > found = true; > break; > @@ -159,14 +168,15 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > if (found) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > + > pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight }, > { bdsWidth, bdsIntHeight }, gdc }); > return; > } > } else { > ifHeight = utils::alignUp(iif.height, IF_ALIGN_H); > - while (ifHeight > minIFHeight && > - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { > + while (ifHeight > minIFHeight && ifHeight / bdsSF >= minBDSHeight) { > + > bdsHeight = ifHeight / bdsSF; > if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > @@ -183,8 +193,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > } > } > > -void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, > - float bdsSF) > +void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, float bdsSF) > { > unsigned int minBDSWidth = gdc.width + FILTER_H * 2; > > @@ -218,15 +227,14 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, > Size calculateGDC(ImgUDevice::Pipe *pipe) > { > const Size &in = pipe->input; > - const Size &main = pipe->output; > + const Size &main = pipe->main; > const Size &vf = pipe->viewfinder; > Size gdc; > > if (!vf.isNull()) { > gdc.width = main.width; > > - float ratio = static_cast<float>(main.width * vf.height) / > - static_cast<float>(vf.width); > + float ratio = (main.width * vf.height) / static_cast<float>(vf.width); > gdc.height = std::max(static_cast<float>(main.height), ratio); > > return gdc; > @@ -237,14 +245,13 @@ Size calculateGDC(ImgUDevice::Pipe *pipe) > return gdc; > } > > - float totalSF = static_cast<float>(in.width) / > - static_cast<float>(main.width); > + float totalSF = static_cast<float>(in.width) / main.width; > float bdsSF = totalSF > 2 ? 2 : 1; > float yuvSF = totalSF / bdsSF; > float sf = findScaleFactor(yuvSF, gdcScalingFactors); > > - gdc.width = static_cast<float>(main.width) * sf; > - gdc.height = static_cast<float>(main.height) * sf; > + gdc.width = main.width * sf; > + gdc.height = main.height * sf; > > return gdc; > } > @@ -259,6 +266,7 @@ FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe) > float ifCropH = static_cast<float>(in.height - pipe.iif.height); > float gdcCropW = static_cast<float>(pipe.bds.width - pipe.gdc.width) * pipe.bds_sf; > float gdcCropH = static_cast<float>(pipe.bds.height - pipe.gdc.height) * pipe.bds_sf; > + > fov.w = (inW - (ifCropW + gdcCropW)) / inW; > fov.h = (inH - (ifCropH + gdcCropH)) / inH; > > @@ -304,11 +312,11 @@ FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe) > * \var Pipe::input > * \brief The input image size > * > - * \var Pipe::output > + * \var Pipe::main > * \brief The requested main output size > * > * \var Pipe::viewfinder > - * \brief The requested viewfinder size > + * \brief The requested viewfinder output size > */ > > /** > @@ -378,7 +386,7 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe) > > LOG(IPU3, Debug) << "Calculating pipe configuration for: "; > LOG(IPU3, Debug) << "input: " << pipe->input.toString(); > - LOG(IPU3, Debug) << "main: " << pipe->output.toString(); > + LOG(IPU3, Debug) << "main: " << pipe->main.toString(); > LOG(IPU3, Debug) << "vf: " << pipe->viewfinder.toString(); > > const Size &in = pipe->input; > @@ -387,9 +395,9 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *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) / > - static_cast<float>(gdc.width); > + float bdsSF = static_cast<float>(in.width) / gdc.width; > float sf = findScaleFactor(bdsSF, bdsScalingFactors, true); > + > while (ifWidth >= minIfWidth) { > Size iif{ ifWidth, ifHeight }; > calculateBDS(pipe, iif, gdc, sf); > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > index ff8dab7d645c..c73ac5a5a37c 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.h > +++ b/src/libcamera/pipeline/ipu3/imgu.h > @@ -37,7 +37,7 @@ public: > > struct Pipe { > Size input; > - Size output; > + Size main; > Size viewfinder; > }; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 06afc7d79dac..161a88da8497 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -225,11 +225,12 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > * margins from the CIO2 output frame size. > * > * The ImgU outputs needs to be strictly smaller than > - * the CIO2 output frame and rounded down to 64 > - * pixels in width and 32 pixels in height. This comes > - * from inspecting the pipe configuration script results > - * and the available suggested configurations in the > - * ChromeOS BSP .xml camera tuning files. > + * the CIO2 output frame and rounded down to 64 pixels > + * in width and 32 pixels in height. This assumption > + * comes from inspecting the pipe configuration script > + * results and the available suggested configurations in > + * the ChromeOS BSP .xml camera tuning files and shall > + * be validated. > * > * \todo Clarify what are the hardware constraints > * that require this alignements, if any. It might > @@ -267,9 +268,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > cfg->setStream(const_cast<Stream *>(&data_->outStream_)); > mainOutputAvailable = false; > > - pipe.output = cfg->size; > + pipe.main = cfg->size; > if (yuvCount == 1) > - pipe.viewfinder = pipe.output; > + pipe.viewfinder = pipe.main; > > LOG(IPU3, Debug) << "Assigned " << cfg->toString() > << " to the main output"; > @@ -333,8 +334,8 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > * to the ImgU maximum output size) and aligned down to > * the required frame margin. > * > - * The alignment constraints should be clarified > - * as per the todo item in validate(). > + * \todo Clarify the alignment constraints as exaplained > + * in validate() > */ > size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE); > size.width = utils::alignDown(size.width - 1, > > Thanks > j > > Jacopo Mondi (19): > libcamera: ipu3: Rename mbusCodesToInfo > libcamera: utils: Add alignUp and alignDown functions > libcamera: geometry: Add isNull() function to Rectangle class > libcamera: ipu3: Remove streams from generateConfiguration > libcamera: ipu3: Make sure the config is valid > libcamera: ipu3: cio2: Report format and sizes > libcamera: ipu3: Do not overwrite StreamConfiguration > libcamera: ipu3: Report StreamFormats > libcamera: ipu3: Remove initialization of Size > libcamera: ipu3: Validate the stream combination > libcamera: camera: Zero streams before validate() > libcamera: ipu3: cio2: Add a const sensor() method > libcamera: ipu3: Adjust and assign streams in validate() > libcamera: ipu3: Remove streams from IPU3CameraConfiguration > libcamera: ipu3: Remove camera_ from IPU3CameraConfiguration > libcamera: ipu3: imgu: Calculate ImgU pipe configuration > libcamera: ipu3: Validate the pipe configuration > libcamera: ipu3: Configure ImgU with the computed parameters > libcamera: ipu3: imgu: Rename configureInput() > > include/libcamera/geometry.h | 1 + > include/libcamera/internal/utils.h | 10 + > src/libcamera/camera.cpp | 4 +- > src/libcamera/geometry.cpp | 6 + > src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++- > src/libcamera/pipeline/ipu3/cio2.h | 6 + > src/libcamera/pipeline/ipu3/imgu. | 0 > src/libcamera/pipeline/ipu3/imgu.cpp | 386 +++++++++++++++++++++++- > src/libcamera/pipeline/ipu3/imgu.h | 22 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 424 +++++++++++++-------------- > src/libcamera/utils.cpp | 16 + > test/geometry.cpp | 14 + > test/utils.cpp | 11 + > 13 files changed, 714 insertions(+), 240 deletions(-) > create mode 100644 src/libcamera/pipeline/ipu3/imgu. > > -- > 2.27.0 > > _______________________________________________ > 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 a3ea3eabe318..cf26980c75f1 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -25,6 +25,14 @@ LOG_DECLARE_CATEGORY(IPU3) namespace { +/* + * The procedure to calculate the ImgU pipe configuration has been ported + * from the pipe_config.py python script, available at: + * https://github.com/intel/intel-ipu3-pipecfg + * at revision: 61e83f2f7606 ("Add more information into README") + */ + static constexpr unsigned int FILTER_H = 4; static constexpr unsigned int IF_ALIGN_W = 2; @@ -103,10 +111,8 @@ float findScaleFactor(float sf, const std::vector<float> &range, bool isSameRatio(const Size &in, const Size &out) { - float inRatio = static_cast<float>(in.width) / - static_cast<float>(in.height); - float outRatio = static_cast<float>(out.width) / - static_cast<float>(out.height); + float inRatio = static_cast<float>(in.width) / in.height; + float outRatio = static_cast<float>(out.width) / out.height; if (std::abs(inRatio - outRatio) > 0.1) return false; @@ -123,16 +129,18 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc float bdsHeight; if (!isSameRatio(pipe->input, gdc)) { - bool found = false; - float estIFHeight = static_cast<float>(iif.width * gdc.height) / + float estIFHeight = (iif.width * gdc.height) / static_cast<float>(gdc.width); estIFHeight = utils::clamp<float>(estIFHeight, minIFHeight, iif.height); + bool found = false; + ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); - while (ifHeight >= minIFHeight && - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { - bdsHeight = static_cast<float>(ifHeight) / bdsSF; + while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) { + + bdsHeight = ifHeight / bdsSF; if (std::fmod(bdsHeight, 1.0) == 0) { unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); + if (!(bdsIntHeight % BDS_ALIGN_H)) { found = true; break; @@ -143,11 +151,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc } ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H); - while (ifHeight <= iif.height && - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { - bdsHeight = static_cast<float>(ifHeight) / bdsSF; + while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { + + bdsHeight = ifHeight / bdsSF; if (std::fmod(bdsHeight, 1.0) == 0) { unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); + if (!(bdsIntHeight % BDS_ALIGN_H)) { found = true; break; @@ -159,14 +168,15 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc if (found) { unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); + pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight }, { bdsWidth, bdsIntHeight }, gdc }); return; } } else { ifHeight = utils::alignUp(iif.height, IF_ALIGN_H); - while (ifHeight > minIFHeight && - static_cast<float>(ifHeight) / bdsSF >= minBDSHeight) { + while (ifHeight > minIFHeight && ifHeight / bdsSF >= minBDSHeight) { + bdsHeight = ifHeight / bdsSF; if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) { unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); @@ -183,8 +193,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc } } -void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, - float bdsSF) +void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, float bdsSF) { unsigned int minBDSWidth = gdc.width + FILTER_H * 2; @@ -218,15 +227,14 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, Size calculateGDC(ImgUDevice::Pipe *pipe) { const Size &in = pipe->input; - const Size &main = pipe->output; + const Size &main = pipe->main; const Size &vf = pipe->viewfinder; Size gdc; if (!vf.isNull()) { gdc.width = main.width; - float ratio = static_cast<float>(main.width * vf.height) / - static_cast<float>(vf.width); + float ratio = (main.width * vf.height) / static_cast<float>(vf.width); gdc.height = std::max(static_cast<float>(main.height), ratio); return gdc; @@ -237,14 +245,13 @@ Size calculateGDC(ImgUDevice::Pipe *pipe) return gdc; } - float totalSF = static_cast<float>(in.width) / - static_cast<float>(main.width); + float totalSF = static_cast<float>(in.width) / main.width; float bdsSF = totalSF > 2 ? 2 : 1; float yuvSF = totalSF / bdsSF; float sf = findScaleFactor(yuvSF, gdcScalingFactors); - gdc.width = static_cast<float>(main.width) * sf; - gdc.height = static_cast<float>(main.height) * sf; + gdc.width = main.width * sf; + gdc.height = main.height * sf; return gdc; } @@ -259,6 +266,7 @@ FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe) float ifCropH = static_cast<float>(in.height - pipe.iif.height); float gdcCropW = static_cast<float>(pipe.bds.width - pipe.gdc.width) * pipe.bds_sf; float gdcCropH = static_cast<float>(pipe.bds.height - pipe.gdc.height) * pipe.bds_sf; + fov.w = (inW - (ifCropW + gdcCropW)) / inW; fov.h = (inH - (ifCropH + gdcCropH)) / inH; @@ -304,11 +312,11 @@ FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe) * \var Pipe::input * \brief The input image size * - * \var Pipe::output + * \var Pipe::main * \brief The requested main output size * * \var Pipe::viewfinder - * \brief The requested viewfinder size + * \brief The requested viewfinder output size */ /** @@ -378,7 +386,7 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe) LOG(IPU3, Debug) << "Calculating pipe configuration for: "; LOG(IPU3, Debug) << "input: " << pipe->input.toString(); - LOG(IPU3, Debug) << "main: " << pipe->output.toString(); + LOG(IPU3, Debug) << "main: " << pipe->main.toString(); LOG(IPU3, Debug) << "vf: " << pipe->viewfinder.toString(); const Size &in = pipe->input; @@ -387,9 +395,9 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *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) / - static_cast<float>(gdc.width); + float bdsSF = static_cast<float>(in.width) / gdc.width; float sf = findScaleFactor(bdsSF, bdsScalingFactors, true); + while (ifWidth >= minIfWidth) { Size iif{ ifWidth, ifHeight }; calculateBDS(pipe, iif, gdc, sf); diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h index ff8dab7d645c..c73ac5a5a37c 100644 --- a/src/libcamera/pipeline/ipu3/imgu.h +++ b/src/libcamera/pipeline/ipu3/imgu.h @@ -37,7 +37,7 @@ public: struct Pipe { Size input; - Size output; + Size main; Size viewfinder; }; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 06afc7d79dac..161a88da8497 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -225,11 +225,12 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() * margins from the CIO2 output frame size. * * The ImgU outputs needs to be strictly smaller than - * the CIO2 output frame and rounded down to 64 - * pixels in width and 32 pixels in height. This comes - * from inspecting the pipe configuration script results - * and the available suggested configurations in the - * ChromeOS BSP .xml camera tuning files. + * the CIO2 output frame and rounded down to 64 pixels + * in width and 32 pixels in height. This assumption + * comes from inspecting the pipe configuration script + * results and the available suggested configurations in + * the ChromeOS BSP .xml camera tuning files and shall + * be validated. * * \todo Clarify what are the hardware constraints * that require this alignements, if any. It might @@ -267,9 +268,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() cfg->setStream(const_cast<Stream *>(&data_->outStream_)); mainOutputAvailable = false; - pipe.output = cfg->size; + pipe.main = cfg->size; if (yuvCount == 1) - pipe.viewfinder = pipe.output; + pipe.viewfinder = pipe.main; LOG(IPU3, Debug) << "Assigned " << cfg->toString() << " to the main output"; @@ -333,8 +334,8 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, * to the ImgU maximum output size) and aligned down to * the required frame margin. * - * The alignment constraints should be clarified - * as per the todo item in validate(). + * \todo Clarify the alignment constraints as exaplained + * in validate() */ size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE); size.width = utils::alignDown(size.width - 1,