[libcamera-devel] [PATCH v5 00/19] ipu3: rework pipe confiuguration
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Aug 3 11:20:19 CEST 2020
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list