[libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate constraints
Nick Hollinghurst
nick.hollinghurst at raspberrypi.com
Wed Jan 3 15:17:15 CET 2024
Hi all,
> From: David Plowman <david.plowman at raspberrypi.com>
> Date: Tue, 2 Jan 2024 at 12:15
> Subject: Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line
> rate constraints
> To: Naushir Patuck <naush at raspberrypi.com>
> Cc: <libcamera-devel at lists.libcamera.org>
>
>
> 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
LGTM
The constraint is on the rate of pixels going from CSI2 peripheral to
ISP-FE (400Mpix/s, plus tiny overheads per scanline, for which
380Mpix/s is a conservative bound).
There is a 64kbit data FIFO before the bottleneck, which means that in
all reasonable cases the constraint applies at a timescale >= 1
scanline, so adding horizontal blanking can prevent loss.
If the backlog were to grow beyond 64kbit during a single scanline,
there could still be loss. This could happen using 4 lanes at 1.5Gbps
at 10bpp with frames wider than ~16,000 pixels. We don't expect to
reach that with any known camera, so the change is good.
Nick
Reviewed-by: Nick Hollinghurst <nick.hollinghurst at raspberrypi.com>
> > ---
> > 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
> >
More information about the libcamera-devel
mailing list