[libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate constraints

[David Plowman]

Hi Naush

Thanks for the patch.

On Tue, 19 Dec 2023 at 12:57, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Advertise hardware constraints on the pixel processing rate through the
> Controller::HardwareConfig structure. When calculating the minimum line
> length during a configure() operation, ensure that we don't exceed this
> constraint.
>
> If we do exceed the hardware constraints, increase the modes's minimum
> line length so the pixel processing rate falls below the hardware limit.
> If this is not possible, throw a loud error message in the logs.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

The change and code here seem reasonable to me. I guess the only minor
concern is to understand exactly what the constraint really is. Is it
just about the line time, or is there a constraint on some number of
blanking pixels? Or maybe the latter is equivalent to the former by
virtue of the fifos?

So it's an "OK" from me, but we should probably ask Nick for chapter and verse!

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!
David

> ---
>  src/ipa/rpi/common/ipa_base.cpp       | 26 ++++++++++++++++++++++++++
>  src/ipa/rpi/controller/controller.cpp |  3 +++
>  src/ipa/rpi/controller/controller.h   |  2 ++
>  3 files changed, 31 insertions(+)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 6ac9d5de2f88..419f19f120da 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -531,6 +531,32 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)
>         mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);
>         mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);
>
> +       /*
> +        * Ensure that the maximum pixel processing rate does not exceed the ISP
> +        * hardware capabilities. If it does, try adjusting the minimum line
> +        * length to compensate if possible.
> +        */
> +       Duration minPixelTime = controller_.getHardwareConfig().minPixelProcessingTime;
> +       Duration pixelTime = mode_.minLineLength / mode_.width;
> +       if (minPixelTime && pixelTime < minPixelTime) {
> +               Duration adjustedLineLength = minPixelTime * mode_.width;
> +               if (adjustedLineLength <= mode_.maxLineLength) {
> +                       LOG(IPARPI, Info)
> +                               << "Adjusting mode minimum line length from " << mode_.minLineLength
> +                               << " to " << adjustedLineLength << " because of HW constraints.";
> +                       mode_.minLineLength = adjustedLineLength;
> +               } else {
> +                       LOG(IPARPI, Error)
> +                               << "Sensor minimum line length of " << pixelTime * mode_.width
> +                               << " (" << 1us / pixelTime << " MPix/s)"
> +                               << " is below the minimum allowable HW limit of " << minPixelTime * mode_.width
> +                               << " (" << 1us / minPixelTime << " MPix/s) ";
> +                       LOG(IPARPI, Error)
> +                               << "THIS WILL CAUSE IMAGE CORRUPTION!!! "
> +                               << "Please update the driver to allow more horizontal blanking control.";
> +               }
> +       }
> +
>         /*
>          * Set the frame length limits for the mode to ensure exposure and
>          * framerate calculations are clipped appropriately.
> diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> index e62becd87e85..f81edf769736 100644
> --- a/src/ipa/rpi/controller/controller.cpp
> +++ b/src/ipa/rpi/controller/controller.cpp
> @@ -17,6 +17,7 @@
>
>  using namespace RPiController;
>  using namespace libcamera;
> +using namespace std::literals::chrono_literals;
>
>  LOG_DEFINE_CATEGORY(RPiController)
>
> @@ -37,6 +38,7 @@ static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap
>                         .numGammaPoints = 33,
>                         .pipelineWidth = 13,
>                         .statsInline = false,
> +                       .minPixelProcessingTime = 0s,
>                 }
>         },
>         {
> @@ -51,6 +53,7 @@ static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap
>                         .numGammaPoints = 64,
>                         .pipelineWidth = 16,
>                         .statsInline = true,
> +                       .minPixelProcessingTime = 1.0us / 380, /* 380 MPix/s */
>                 }
>         },
>  };
> diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h
> index 6e5f595284fd..170aea740789 100644
> --- a/src/ipa/rpi/controller/controller.h
> +++ b/src/ipa/rpi/controller/controller.h
> @@ -15,6 +15,7 @@
>  #include <vector>
>  #include <string>
>
> +#include <libcamera/base/utils.h>
>  #include "libcamera/internal/yaml_parser.h"
>
>  #include "camera_mode.h"
> @@ -47,6 +48,7 @@ public:
>                 unsigned int numGammaPoints;
>                 unsigned int pipelineWidth;
>                 bool statsInline;
> +               libcamera::utils::Duration minPixelProcessingTime;
>         };
>
>         Controller();
> --
> 2.34.1
>