[libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate constraints
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jan 4 23:20:27 CET 2024
On Thu, Jan 04, 2024 at 08:28:10AM +0000, Naushir Patuck wrote:
> On Wed, 3 Jan 2024 at 18:04, Laurent Pinchart via libcamera-devel wrote:
> > 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 ?
>
> Sure I can add that.
>
> > > 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 ?
>
> I would prefer an info message personally so it shows up in the logs
> by default. This will help when users ask why their camera is not
> running as fast as they want.
OK.
> > > > > + 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 ?
>
> My preference would be not to, and allow the user to continue running.
> The reason is there are mitigations we might ask users to try out
> (e.g. upclocking RP1) in certain scenarios. Having this not fail will
> allow the user to try such things without having to recompile
> libcamera (but of course, the error message will still be displayed).
Good point.
By the way, have you considered a solution where the upclocking would be
done automatically ?
> > > > > + }
> > > > > + }
> > > > > +
> > > > > /*
> > > > > * 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