[libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate constraints
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 3 19:04:13 CET 2024
On Wed, Jan 03, 2024 at 02:17:15PM +0000, Nick Hollinghurst via libcamera-devel wrote:
> On Tue, 2 Jan 2024 at 12:15, David Plowman wrote:
> > On Tue, 19 Dec 2023 at 12:57, Naushir Patuck via libcamera-devel 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>
>
> 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.
This is a useful explanation, could it be added to the comments in the
source code ?
> 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.";
Does this need to be an info message or would debug be enough ?
> > > + mode_.minLineLength = adjustedLineLength;
> > > + } else {
> > > + LOG(IPARPI, Error)
> > > + << "Sensor minimum line length of " << pixelTime * mode_.width
> > > + << " (" << 1us / pixelTime << " MPix/s)"
Technically, as pixelTime is a time, 1us / pixelTime is a unit-less
value. It could be written as '1 / pixelTime.get<std::micro>()', but
adding the rouding, that would give
std::lround(1 / pixelTime.get<std::micro>())
which I suppose isn't very nice to read. I'll stop being pedantic here
:-)
> > > + << " is below the minimum allowable HW limit of " << minPixelTime * mode_.width
I would write "ISP limit" instead of "HW limit" to make it clear where
the constraint comes from.
Please add a line break before '<< minPixelTime'.
> > > + << " (" << 1us / minPixelTime << " MPix/s) ";
> > > + LOG(IPARPI, Error)
> > > + << "THIS WILL CAUSE IMAGE CORRUPTION!!! "
> > > + << "Please update the driver to allow more horizontal blanking control.";
Similarly, "Please update the camera sensor driver ...".
As corruption is pretty much a given, should we error out ?
> > > + }
> > > + }
> > > +
> > > /*
> > > * 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();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list