[PATCH 3/3] rkisp1: Honor the FrameDurationLimits control
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 28 02:46:12 CET 2024
On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:
> On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:
> > From: Paul Elder <paul.elder at ideasonboard.com>
> >
> > Add support to rkisp1 for controlling the framerate via the
> > FrameDurationLimits control.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> > src/ipa/rkisp1/algorithms/agc.cpp | 52 ++++++++++++++++++------
> > src/ipa/rkisp1/ipa_context.cpp | 16 +++++++-
> > src/ipa/rkisp1/ipa_context.h | 4 ++
> > src/ipa/rkisp1/rkisp1.cpp | 2 +
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++++
> > 5 files changed, 68 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index e23ab120b3e2..088ecf42c480 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >
> > /* Limit the frame duration to match current initialisation */
> > ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> > + context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
> > context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> >
> > /*
> > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,
> >
> > const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> > if (frameDurationLimits) {
> > - utils::Duration maxFrameDuration =
> > - std::chrono::milliseconds((*frameDurationLimits).back());
> > - agc.maxFrameDuration = maxFrameDuration;
> > + /* Limit the control value to the limits in ControlInfo */
Period at end of sentence.
> > + ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];
> > + int64_t minFrameDuration =
> > + std::clamp((*frameDurationLimits).front(),
You can write
std::clamp(frameDurationLimits->front(),
> > + limits.min().get<int64_t>(),
> > + limits.max().get<int64_t>());
> > + int64_t maxFrameDuration =
> > + std::clamp((*frameDurationLimits).back(),
Same here.
> > + limits.min().get<int64_t>(),
> > + limits.max().get<int64_t>());
> > +
>
> We operate on the assumption the control is well-formed, right ? iow
> that frameDurationLimits[0] < frameDurationLimits[1] ?
>
> > + agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);
> > + agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
> > }
> > + frameContext.agc.minFrameDuration = agc.minFrameDuration;
> > frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
> > }
> >
> > @@ -330,15 +342,8 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > * frameContext.sensor.exposure;
> > metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> > metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> > + metadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get<std::micro>());
> > metadata.set(controls::AeEnable, frameContext.agc.autoEnabled);
> > -
> > - /* \todo Use VBlank value calculated from each frame exposure. */
> > - uint32_t vTotal = context.configuration.sensor.size.height
> > - + context.configuration.sensor.defVBlank;
> > - utils::Duration frameDuration = context.configuration.sensor.lineDuration
> > - * vTotal;
> > - metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> > -
> > metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> > metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> > metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
> > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > return;
> > }
> >
> > + IPACameraSensorInfo &sensorInfo = context.sensorInfo;
const
> > utils::Duration lineDuration = context.configuration.sensor.lineDuration;
> >
> > /*
> > @@ -418,11 +424,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > [](uint32_t x) { return x >> 4; });
> > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> >
> > + /*
> > + * Limit the allowed shutter speeds for the exposure helper based on
> > + * the frame duration limits.
> > + */
I think we should phase out the term "shutter speed" and use "frame
duration" everywhere. I've send an RFC patch.
> > + utils::Duration minShutterSpeed =
> > + std::clamp(frameContext.agc.minFrameDuration,
> > + context.configuration.sensor.minShutterSpeed,
> > + context.configuration.sensor.maxShutterSpeed);
> > utils::Duration maxShutterSpeed =
> > std::clamp(frameContext.agc.maxFrameDuration,
> > context.configuration.sensor.minShutterSpeed,
> > context.configuration.sensor.maxShutterSpeed);
> > - setLimits(context.configuration.sensor.minShutterSpeed,
> > + setLimits(minShutterSpeed,
> > maxShutterSpeed,
> > context.configuration.sensor.minAnalogueGain,
> > context.configuration.sensor.maxAnalogueGain);
> > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > activeState.agc.automatic.exposure = shutterTime / lineDuration;
> > activeState.agc.automatic.gain = aGain;
> >
> > + /*
> > + * Determine what our FrameDuration must be and adapt VBLANK to suit
> > + * this if we need to expand the shutter time calculated to fill the
> > + * remaining time so that we do not run faster than the minimum frame
> > + * duration (maximum requested frame rate) when we have short exposures.
That's hard to read.
> > + */
> > + utils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,
> > + shutterTime);
Sensors usually have a minimum margin between the exposure time and
frame duration, see frameIntegrationDiff in src/ipa/rpi/cam_helper/
> > +
> > + frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> > +
> > + /* Update accounting for line length quantization. */
> > + frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> > +
> > fillMetadata(context, frameContext, metadata);
> > expMeans_ = {};
> > }
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 14d0c02a2b32..c5eb0d64ec29 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {
> > * \var IPAActiveState::agc.meteringMode
> > * \brief Metering mode as set by the AeMeteringMode control
> > *
> > + * \var IPAActiveState::agc.minFrameDuration
> > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > + *
> > * \var IPAActiveState::agc.maxFrameDuration
> > * \brief Maximum frame duration as set by the FrameDurationLimits control
> > */
> > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {
> > * \brief Automatic Gain Control parameters for this frame
> > *
> > * The exposure and gain are provided by the AGC algorithm, and are to be
> > - * applied to the sensor in order to take effect for this frame.
> > + * applied to the sensor in order to take effect for this frame. Additionally
> > + * the vertical blanking period is determined to maintain a consistent frame
> > + * rate matched to the FrameDurationLimits as set by the user.
> > *
> > * \var IPAFrameContext::agc.exposure
> > * \brief Exposure time expressed as a number of lines computed by the algorithm
> > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {
> > *
> > * The gain should be adapted to the sensor specific gain code before applying.
> > *
> > + * \var IPAFrameContext::agc.vblank
> > + * \brief Vertical blanking parameter computed by the algorithm
> > + *
> > * \var IPAFrameContext::agc.autoEnabled
> > * \brief Manual/automatic AGC state as set by the AeEnable control
> > *
> > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {
> > * \var IPAFrameContext::agc.meteringMode
> > * \brief Metering mode as set by the AeMeteringMode control
> > *
> > + * \var IPAFrameContext::agc.minFrameDuration
> > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > + *
> > * \var IPAFrameContext::agc.maxFrameDuration
> > * \brief Maximum frame duration as set by the FrameDurationLimits control
> > *
> > + * \var IPAFrameContext::agc.frameDuration
> > + * \brief The actual FrameDuration used by the algorithm for the frame
> > + *
> > * \var IPAFrameContext::agc.updateMetering
> > * \brief Indicate if new ISP AGC metering parameters need to be applied
> > */
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index e274d9b01e1c..60c4d647f1ef 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -79,6 +79,7 @@ struct IPAActiveState {
> > controls::AeConstraintModeEnum constraintMode;
> > controls::AeExposureModeEnum exposureMode;
> > controls::AeMeteringModeEnum meteringMode;
> > + utils::Duration minFrameDuration;
> > utils::Duration maxFrameDuration;
> > } agc;
> >
> > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {
> > struct {
> > uint32_t exposure;
> > double gain;
> > + uint32_t vblank;
> > bool autoEnabled;
> > controls::AeConstraintModeEnum constraintMode;
> > controls::AeExposureModeEnum exposureMode;
> > controls::AeMeteringModeEnum meteringMode;
> > + utils::Duration minFrameDuration;
> > utils::Duration maxFrameDuration;
> > + utils::Duration frameDuration;
> > bool updateMetering;
> > } agc;
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 47777ece783f..17d56fb4e901 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)
> > IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > uint32_t exposure = frameContext.agc.exposure;
> > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> > + uint32_t vblank = frameContext.agc.vblank;
> >
> > ControlList ctrls(sensorControls_);
> > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > + ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> >
> > setSensorControls.emit(frame, ctrls);
> > }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 42961c120036..1ec12185bb78 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
> > void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> > const ControlList &sensorControls)
> > {
> > + if (frame == 0) {
> > + ControlList controls = sensorControls;
> > + sensor_->setControls(&controls);
> > + return;
> > + }
> > +
>
> I might have missed what is this for :)
It should be explained in the commit message and possibly split to a
separate patch.
> > delayedCtrls_->push(sensorControls);
> > }
> >
> > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > { V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > { V4L2_CID_EXPOSURE, { 2, false } },
> > + { V4L2_CID_VBLANK, { 1, false } },
Time to fix the hardcoded delays (in a separate series of course).
> > };
> >
> > data->delayedCtrls_ =
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list