[libcamera-devel] [PATCH v5 00/19] ipu3: rework pipe confiuguration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 31 17:41:54 CEST 2020
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.
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list