[libcamera-devel] [PATCH RFC] rkisp1: adjust vblank to target framerate
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed May 24 11:04:10 CEST 2023
Hi Benjamin
On Tue, May 23, 2023 at 07:04:43PM +0200, Benjamin Bara via libcamera-devel wrote:
> From: Benjamin Bara <benjamin.bara at skidata.com>
>
> Set the vertical blanking of a sensor accordingly to a targeted
> framerate.
>
> E.g. gst_libcamera_clamp_and_set_frameduration() sets
> FrameDurationLimits to the initControls_, which are then passed to
> Camera::start() (and from there to PipelineHandler::start()).
>
> Example (imx327: 1080p at 25; default/minimum hBlank: 280):
> vBlank = 0.04 * 148500000 / 2200 - 1080 = 1620
> which results to:
> 16390.268661 (25.00 fps) cam0-stream0 seq: 000001 bytesused: 2073600/1036800
> 16390.308661 (25.00 fps) cam0-stream0 seq: 000002 bytesused: 2073600/1036800
>
> When doing this on sensors where the allowed exposure range depends on
> vblank (e.g. on the imx327), the upper exposure limit is increased (
> from 1123 to 2698 in the example above).
>
> As the sensor controls are "cached" in the IPA context, they must be
> updated. This is done by passing the updated values via the start()
> method.
>
> In general, this could be done independently of the IPA.
>
> Signed-off-by: Benjamin Bara <benjamin.bara at skidata.com>
> ---
> Some checks are still missing in this version, but I wanted to check
> first if my approach fits and if this is something worth to look further
> into.
I think the approach is right. So far frame durations set at start()
time are ignored on RkISP1 and this changeset is certainly welcome.
By pieces:
1) Adding sensor controls to IPA::start() I think it's correct
2) Updating vblank in ph before calling IPA::start()
Here I would consider a different approach
The call stack you have implemented is
ph::start() {
compute VBLANK
set VBLANK on sensor
ipa->start(v4l2_controls)
IPA::start(v4l2_controls) {
update exposure limits
setControls(0)
ctrls(EXPOSURE, ...)
ctrls(GAIN)
setSensorControls.emit()
}
setSensorControls()
delayedCtrl_->push(ctrls);
}
This seems convenient, as applying VBLANK on the sensor before
calling the IPA guarantees EXPOSURE is up-to-date with the newly
computed vertical blanking.
I wonder if we shouldn't instead remove the call to setControls(0)
in IPA::start() and return a list of v4l2 controls from
IPA::start() as raspberrypi does so that the new VBLANK EXPOSURE
and GAIN are computed all on the IPA side by re-using
updateControls() which re-computes the limits for the
Camera::controls as well.
Your new call stack would be like
ph::start(ctrls)
ipa->start(ctrls, data->controlInfo_)
IPA::start(ctrls, cameraCtrs)
recompute VBLANK
recompute EXPOSURE
recompute GAIN
setSensorCtrl.emit(sensorCtrls)
setSensorControls()
delayedCtrl_->push()
updateControlInfo(cameraCtrls)
If I'm not mistaken, with your current implementation the
Camera::controls limits are not updated after start(), right ?
Assume you set a very short FrameDurationsLimit and that
controls::Exposure needs to be shrink because of that. Am I wrong
that after start() this will not be refelected in
Camera::controls::Exposure ? Could you quickly test it, it
shouldn't be too hard, just give gst a very high 'framerate' which
requires shrinking the exposure time..
The only drawback I see with my proposal is that the
re-computation of the EXPOSURE v4l2 control limits has to be done
manually in the IPA instead of relaying on it being done by the
driver when setting a new VBLANK as per your current
implementation.
What do you think ?
A few minor style issues below
>
> I also saw that the getBlanking() method inside of the rpi IPA is doing
> something similar, but I guess this can be done independent of the
> sensor and IPA.
>
> Many thanks and best regards,
> Benjamin
> ---
> include/libcamera/ipa/rkisp1.mojom | 2 +-
> src/ipa/rkisp1/rkisp1.cpp | 18 ++++++++++++++--
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 +++++++++++++++++++++++++++++++-
> 3 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 1009e970..0e0df9e8 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -19,7 +19,7 @@ interface IPARkISP1Interface {
> libcamera.IPACameraSensorInfo sensorInfo,
> libcamera.ControlInfoMap sensorControls)
> => (int32 ret, libcamera.ControlInfoMap ipaControls);
> - start() => (int32 ret);
> + start(libcamera.ControlInfoMap sensorControls) => (int32 ret);
> stop();
>
> configure(IPAConfigInfo configInfo,
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 6544c925..2c092efa 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -53,7 +53,7 @@ public:
> const IPACameraSensorInfo &sensorInfo,
> const ControlInfoMap &sensorControls,
> ControlInfoMap *ipaControls) override;
> - int start() override;
> + int start(const ControlInfoMap &sensorControls) override;
> void stop() override;
>
> int configure(const IPAConfigInfo &ipaConfig,
> @@ -197,8 +197,22 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> return 0;
> }
>
> -int IPARkISP1::start()
> +int IPARkISP1::start(const ControlInfoMap &sensorControls)
> {
> + /*
> + * it might be possible that VBLANK has been changed and that this has
> + * an impact on the exposure limits. therefore re-set them here.
> + */
> + const auto itExp = sensorControls.find(V4L2_CID_EXPOSURE);
> + int32_t minExposure = itExp->second.min().get<int32_t>();
> + int32_t maxExposure = itExp->second.max().get<int32_t>();
> + context_.configuration.sensor.minShutterSpeed =
> + minExposure * context_.configuration.sensor.lineDuration;
> + context_.configuration.sensor.maxShutterSpeed =
> + maxExposure * context_.configuration.sensor.lineDuration;
> + LOG(IPARkISP1, Debug)
> + << "Exposure: [" << minExposure << ", " << maxExposure << "]";
> +
> setControls(0);
>
> return 0;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8a30fe06..f9b3a3f7 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -914,12 +914,47 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> RkISP1CameraData *data = cameraData(camera);
> int ret;
>
> + /*
> + * \todo Move this to a IPA-indepent location where a camera sensor
> + * instance is available and the targeted frame duration is known.
> + * Additionally, the IPA's sensor controls must be set accordingly.
> + */
> + auto frameDurations = controls->get(controls::FrameDurationLimits);
> + if (frameDurations && frameDurations->size() == 2) {
> + ControlList sensorControls = data->sensor_->controls();
> + ControlList ctrls;
> + IPACameraSensorInfo sensorInfo;
> + if (data->sensor_->sensorInfo(&sensorInfo)) {
> + LOG(RkISP1, Error) << "couldn't fetch sensor info";
> + }
No braces for single line if statements. Also this should probably be
a fatal error
> +
> + /*
> + * setup vertical blanking for target frame rate:
> + * frameWidth = width + hBlank
> + * frameHeight = height + vBlank
> + * frameDuration = frameWidth * frameHeight / pixelRate
> + * =>
> + * vBlank = frameDuration [us] * pixelRate / frameWidth - height
> + */
> + uint32_t frameWidth = sensorInfo.minLineLength;
> + uint32_t height = sensorInfo.outputSize.height;
> + uint64_t pixelRate = sensorInfo.pixelRate;
> + uint32_t maxFrameDuration = (*frameDurations)[1];
> + int32_t vBlank = maxFrameDuration * pixelRate / (frameWidth * 1000000) - height;
> + LOG(RkISP1, Debug) << "Setting VBLANK to " << vBlank;
> + ctrls.set(V4L2_CID_VBLANK, vBlank);
> + data->sensor_->setControls(&ctrls);
> + data->sensor_->updateControlInfo();
> + } else {
> + LOG(RkISP1, Debug) << "Skip setting VBLANK";
> + }
This could be skipped. Maybe we should make sure
frameDurations->size() == 2 and fail if that's not the case. But if
!frameDurations I don't think we need a debug printout
Thanks
j
> +
> /* Allocate buffers for internal pipeline usage. */
> ret = allocateBuffers(camera);
> if (ret)
> return ret;
>
> - ret = data->ipa_->start();
> + ret = data->ipa_->start(data->sensor_->controls());
> if (ret) {
> freeBuffers(camera);
> LOG(RkISP1, Error)
>
> ---
> base-commit: e8fccaea46b9e545282cd37d54b1acb168608a46
> change-id: 20230523-rkisp1-vblank-3ad862689e4a
>
> Best regards,
> --
> Benjamin Bara <benjamin.bara at skidata.com>
>
More information about the libcamera-devel
mailing list