[libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration to 30 FPS

[Jacopo Mondi]

Hi Umang,

On Mon, Sep 06, 2021 at 04:33:10PM +0530, Umang Jain wrote:
> Hi Jacopo,
>
> I have a few questions regarding the todos
>
> On 8/27/21 5:37 PM, Jacopo Mondi wrote:
> > Limit the IPU3 frame rate to 30 FPS.
> >
> > The reason to do is to bring the IPU3 IPA in par with the Intel
> > HAL implementation on IPU3 platform, where 30FPS is the frame rate used
> > to perform quality tuning in the closed-source IPA module and has been
> > validated as the most efficient rate for the power/performace budget.
> >
> > Compute the vertical blanking to maintain such frame rate and configure
> > the sensor with that.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >   src/ipa/ipu3/ipu3.cpp | 37 ++++++++++++++++++++++++++++++++-----
> >   1 file changed, 32 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index fc5f69ed5ddc..0e5d5e479e20 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -183,7 +183,7 @@ private:
> >   	IPACameraSensorInfo sensorInfo_;
> >   	/* Camera sensor controls. */
> > -	uint32_t defVBlank_;
> > +	uint32_t vBlank_;
> >   	uint32_t exposure_;
> >   	uint32_t minExposure_;
> >   	uint32_t maxExposure_;
> > @@ -257,10 +257,39 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
> >   		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> >   	}
> > +	/*
> > +	 * Cap minimum frame duration to 30FPS.
> > +	 *
> > +	 * 30 FPS has been validated in the closed source Intel 3A module as the
> > +	 * most opportune frame rate for quality tuning, and power
> > +	 * vs performances budget on Intel IPU3.
> > +	 *
> > +	 * Reduce the minimum achievable frame rate to 30 FPS and compute the
> > +	 * vertical blanking to maintain that rate.
> > +	 */
> > +	int64_t *minFrameDuration = &frameDurations[0];
> > +	if (*minFrameDuration < 1e6 / 30.0)
> > +		*minFrameDuration = 1e6 / 30.0;
> > +
> >   	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
> >   							       frameDurations[1],
> >   							       frameDurations[2]);
> > +	/*
> > +	 * Adjust the vertical blanking to obtain the desired frame duration.
> > +	 *
> > +	 * Assume a fixed line length as horizontal blanking is seldom
> > +	 * controllable.
> > +	 *
> > +	 * \todo Support making this overridable by the application through
> > +	 * controls::FrameDuration.
>
>
> In general, are applications poised to alter frame duration ? I think
> applications are poised to request manual exposure control, correct me if I
> am wrong! Hence, I don't understand the concept of overridable
> controls::FrameDuration as such.
>

>From control_id.yaml

        When provided by applications, the control specifies the sensor frame
        duration interval the pipeline has to use. This limits the largest
        exposure time the sensor can use. For example, if a maximum frame
        duration of 33ms is requested (corresponding to 30 frames per second),
        the sensor will not be able to raise the exposure time above 33ms.
        A fixed frame duration is achieved by setting the minimum and maximum
        values to be the same. Setting both values to 0 reverts to using the
        IPA provided defaults.


> > +	 *
> > +	 * \todo Clamp exposure to frame duration.
>
>
> If a manual exposure is requested, will the clamping be in effect? In my

>From control_ids.yaml

        The maximum frame duration provides the absolute limit to the shutter
        speed computed by the AE algorithm and it overrides any exposure mode
        setting specified with controls::AeExposureMode. Similarly, when a
        manual exposure time is set through controls::ExposureTime, it also
        gets clipped to the limits set by this control.

You know, I feel the hardest part in defining controls is not their
definition in isolation but defining how they interact together. The
number of \todo entries in controls_ids.yaml should suggest that to
you as well.

As the AEGC related definitions are in rework, this might change as
well. So far it matches what has been implemented by RPi, where the
exposure time (being it computed by the AEGC or manually set) is
always clamped in the frame duration limits.


> head, the broader concept is - manual exposure -> set exposure (which
> requires new vblank), calculate new vblank -> set new vblank (which is to
> (-) \todo  of in patch, see below).
>
> > +	 */
> > +	vBlank_ = *minFrameDuration * (sensorInfo.pixelRate / 1000000U);
> > +	vBlank_ /= lineLength;
> > +	vBlank_ -= sensorInfo.outputSize.height;
> > +
> >   	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> >   }
> > @@ -399,8 +428,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >   	maxGain_ = itGain->second.max().get<int32_t>();
> >   	gain_ = minGain_;
> > -	defVBlank_ = itVBlank->second.def().get<int32_t>();
> > -
> >   	/* Clean context at configuration */
> >   	context_ = {};
> > @@ -511,8 +538,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >   	setControls(frame);
> > -	/* \todo Use VBlank value calculated from each frame exposure. */
>
>
> I think what I am trying to ask is, this patch deletes one \todo, and
> introduces two other \todos. The descriptions  of new \todos are confusing
> to me or maybe they need some general discussion.
>
> This previous todo, seems to comply with Intel HAL i.e. The Vblank is set
> from exposure's results. (See SyncManager::applySensorParams and 
> SensorHwOp::setFrameDuration). These use exposure's results derived from
> IPA.

I think the vblank calculated by the AEGC algorithm shuld be in the
limits defined by the frame durations.

It didn' get what you mean with:
"These use exposure's results derived from  IPA"

Does it mean the Intel IPA adaption later receives the exposure
values from a black box and comptes frame duration limit from there ?
It doesn't comply with our definition of how frame duration and the
AEGC algorithm interact, but I suspect we cannot do much about it.

As long as the definition of how our controls interact doesn't change, and
should changes for a good reason, I think the open IPA design should behave
according to what is there prescribed.

I'm cutting a lot of corners both in the definition of how AEGC and
durations interact and in considering corner cases, but as reference have
a look at the RPi camera helper behaves:

	/*
	 * Limit the exposure to the maximum frame duration requested, and
	 * re-calculate if it has been clipped.
	 */
	exposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);
	exposure = Exposure(exposureLines);

	/* Limit the vblank to the range allowed by the frame length limits. */
	vblank = std::clamp(exposureLines + frameIntegrationDiff_,
			    frameLengthMin, frameLengthMax) - mode_.height;
	return vblank;
}

>
>
> > -	int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> > +	int64_t frameDuration = sensorInfo_.lineLength * (vBlank_ + sensorInfo_.outputSize.height) /
> >   				(sensorInfo_.pixelRate / 1e6);
> >   	ctrls.set(controls::FrameDuration, frameDuration);
> > @@ -534,6 +560,7 @@ void IPAIPU3::setControls(unsigned int frame)
> >   	ControlList ctrls(ctrls_);
> >   	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_));
> >   	op.controls = ctrls;
> >   	queueFrameAction.emit(frame, op);