[libcamera-devel] [PATCH RFC] rkisp1: adjust vblank to target framerate
Mikhail Rudenko
mike.rudenko at gmail.com
Thu Dec 14 17:00:50 CET 2023
Hi Jacopo, Benjamin,
I'd like to work on this to get it merged. I'm rather new to libcamera
code base, so have a couple of questions (see below).
On 2023-05-24 at 11:04 +02, Jacopo Mondi via libcamera-devel <libcamera-devel at lists.libcamera.org> wrote:
> 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.
I've got a couple questions regarding your proposal:
1. Is it okay to start with setting frame duration in start() only (and
thus scratching the gstreamer itch), or is it necessary to handle it on
per-frame basis too?
2. How could we recompute EXPOSURE limits in ipa, without actually
setting VBLANK on sensor subdev and querying EXPOSURE limits from
kernel? Looks like rpi ipa tries to duplicate kernel logic in its
CamHelper (frameIntegrationDiff constant). Should we do something like
that in CameraSensorHelper too, or is there a better way?
>
> 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>
>>
--
Best regards,
Mikhail Rudenko
More information about the libcamera-devel
mailing list