| Message ID | 20230523-rkisp1-vblank-v1-1-381c6f025ff6@skidata.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Benjamin On Tue, May 23, 2023 at 07:04:43PM +0200, Benjamin Bara via libcamera-devel wrote: > From: Benjamin Bara <benjamin.bara@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@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@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@skidata.com> >
Hi Jacopo, thanks for the feedback and sorry for the late response. On Wed, 24 May 2023 at 11:04, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > 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. > > If I'm not mistaken, with your current implementation the > Camera::controls limits are not updated after start(), right ? Exactly, they aren't. As I am fairly new to libcamera and so far only used libcamera in combination with gst-launch: Is it possible to change the frame duration after start() is called? Because IMHO, vblank is static, as long as the frame duration is static. Obviously, if the frame duration limit is dynamic after start() is called, then it would make sense to also have vblank recalculated afterwards. Under my assumption of a static frame duration, I guess it would even make sense to put it "before" or outside of the IPA-specific ph::start(), as it is just related to the camera sensor, and independent of the IPA - but I guess start() is the first call to libcamera where the frame durations are actually known. > 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. Yes, I think so too. This needs to be implemented per-sensor in the helper I guess. I skimmed a little bit through the camera sensor drivers and it looks like most of the drivers adapt the v4l2 exposure limits as soon as vblank is changed (except e.g. imx258 or imx415). So I guess at least it seems to be quite reliable. So IMHO, for the "given frame duration limit" case, I guess it just boils down to the question if the limits can change after calling start(). For other use cases, like reducing the frame rate to increase exposure when in saturation (or similar), your suggestion might fit better. What do you think? Thanks and regards, Benjamin
Hi Benjamin On Tue, Jun 06, 2023 at 07:27:12PM +0200, Benjamin Bara via libcamera-devel wrote: > Hi Jacopo, > > thanks for the feedback and sorry for the late response. > > On Wed, 24 May 2023 at 11:04, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > 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. > > > > If I'm not mistaken, with your current implementation the > > Camera::controls limits are not updated after start(), right ? > > Exactly, they aren't. > As I am fairly new to libcamera and so far only used libcamera in > combination with gst-launch: Is it possible to change the frame duration > after start() is called? Because IMHO, vblank is static, as long as the Frame duration is certainly controllable during streaming yes. controls::FrameDurationLimits is a regular control, and like all other controls, it is meant to be set by the application to change the streaming behaviour. It of course needs to be handled in the pipeline and the IPA (something which is missing in RkISP1, in example). Now, if I read it right, the gst element only allows you to control the frame duration at startup, I'm not sure this is a shortcoming of the current implementation or is there anything in gst which prevents to set those control while the "pipeline is rolling", but when it comes to libcamera itself, the intended behaviour is for application to be able to set controls during streaming. If you want to experiment with that, I would start by hacking out the 'cam' test application and set a FrameDurationLimits[] = {x, x} in a Request after the streaming has started. From there the control is received by the pipeline handler and then handed to the IPA. The IPA should set the agc limits (we even have a todo) /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */ and compute a new vblank value to be sent back to the pipeline (in the same way as it now passed back V4L2_CID_EXPOSURE and V4L2_CID_ANALOGUE_GAIN). It's quite some work, but there shouldn't be anything too complex. If you're interested in giving this a go I would be glad to help. > frame duration is static. Obviously, if the frame duration limit is > dynamic after start() is called, then it would make sense to also have > vblank recalculated afterwards. Under my assumption of a static frame > duration, I guess it would even make sense to put it "before" or outside > of the IPA-specific ph::start(), as it is just related to the camera > sensor, and independent of the IPA - but I guess start() is the first > call to libcamera where the frame durations are actually known. > > > 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. > > Yes, I think so too. This needs to be implemented per-sensor in the > helper I guess. I skimmed a little bit through the camera sensor drivers > and it looks like most of the drivers adapt the v4l2 exposure limits as > soon as vblank is changed (except e.g. imx258 or imx415). So I guess at > least it seems to be quite reliable. > > So IMHO, for the "given frame duration limit" case, I guess it just > boils down to the question if the limits can change after calling > start(). For other use cases, like reducing the frame rate to increase > exposure when in saturation (or similar), your suggestion might fit > better. What do you think? > > Thanks and regards, > Benjamin
Hello, On Tue, Jun 06, 2023 at 07:27:12PM +0200, Benjamin Bara via libcamera-devel wrote: > Hi Jacopo, > > thanks for the feedback and sorry for the late response. No need to apologize, or I'll have to do so too :-) Would you mind not stripping the context in e-mail replies ? Doing so makes it more difficult to jump in the middle of the conversation. If there are very large parts that are clearly not relevant for any further discussion on the topic then they can be trimmed, but in general avoiding trimming would be nice. This is a workflow issue, and I don't want to push everybody to comply with my personal preferences, so if it causes any issue for you, please follow the ancient wisdom from [1] and ignore this whole comment. https://objects-us-east-1.dream.io/secrettoeverybody/images/paynoattention.png > On Wed, 24 May 2023 at 11:04, Jacopo Mondi wrote: > > > > 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. > > > > If I'm not mistaken, with your current implementation the > > Camera::controls limits are not updated after start(), right ? > > Exactly, they aren't. > As I am fairly new to libcamera and so far only used libcamera in > combination with gst-launch: Is it possible to change the frame duration > after start() is called? Because IMHO, vblank is static, as long as the > frame duration is static. Obviously, if the frame duration limit is > dynamic after start() is called, then it would make sense to also have > vblank recalculated afterwards. Under my assumption of a static frame > duration, I guess it would even make sense to put it "before" or outside > of the IPA-specific ph::start(), as it is just related to the camera > sensor, and independent of the IPA - but I guess start() is the first > call to libcamera where the frame durations are actually known. The FrameDurationLimits control is meant to be dynamic. That's how it is implemented for Raspberry Pi, and I think we should do the same on other platforms. > > 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. > > Yes, I think so too. This needs to be implemented per-sensor in the > helper I guess. I skimmed a little bit through the camera sensor drivers > and it looks like most of the drivers adapt the v4l2 exposure limits as > soon as vblank is changed (except e.g. imx258 or imx415). So I guess at > least it seems to be quite reliable. I'm *really* annoyed by the V4L2 API here. The fact that we need to modify VBLANK first before changing the exposure, in two separate ioctls, is bad. I can live with it for the time being. The fact that the VBLANK limits change when the sensor output format is changed is really bad. The fact that most sensors have a fixed constraint on the vertical total size, not on vertical blank itself, upsets me. Our troubles don't come from hardware constraints, but from a really bad API design decision. If we exposed the vertical total size instead of the vertical blanking, the maximum value would be fixed, and userspace would be so much simpler. I don't want to live with this anymore, we should really fix it on the kernel side. I'd be happy to treat whoever makes my life less miserable by fixing this issue to dinner at whatever conference we would attend next :-) > So IMHO, for the "given frame duration limit" case, I guess it just > boils down to the question if the limits can change after calling > start(). For other use cases, like reducing the frame rate to increase > exposure when in saturation (or similar), your suggestion might fit > better. What do you think?
Hello, On Wed, Jun 07, 2023 at 10:45:25AM +0200, Jacopo Mondi via libcamera-devel wrote: > On Tue, Jun 06, 2023 at 07:27:12PM +0200, Benjamin Bara via libcamera-devel wrote: > > On Wed, 24 May 2023 at 11:04, Jacopo Mondi wrote: > > > 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. > > > > > > If I'm not mistaken, with your current implementation the > > > Camera::controls limits are not updated after start(), right ? > > > > Exactly, they aren't. > > As I am fairly new to libcamera and so far only used libcamera in > > combination with gst-launch: Is it possible to change the frame duration > > after start() is called? Because IMHO, vblank is static, as long as the > > Frame duration is certainly controllable during streaming yes. > > controls::FrameDurationLimits is a regular control, and like all other > controls, it is meant to be set by the application to change the > streaming behaviour. It of course needs to be handled in the pipeline > and the IPA (something which is missing in RkISP1, in example). > > Now, if I read it right, the gst element only allows you to control > the frame duration at startup, I'm not sure this is a shortcoming of > the current implementation or is there anything in gst which prevents > to set those control while the "pipeline is rolling", but when it > comes to libcamera itself, the intended behaviour is for application > to be able to set controls during streaming. > > If you want to experiment with that, I would start by hacking out the > 'cam' test application and set a FrameDurationLimits[] = {x, x} in a > Request after the streaming has started. No need to hack the application, you can use a capture script ;-) See `cam -h` and src/apps/cam/capture-script.yaml. > From there the control is > received by the pipeline handler and then handed to the IPA. The IPA > should set the agc limits (we even have a todo) > > /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */ > > and compute a new vblank value to be sent back to the pipeline (in the > same way as it now passed back V4L2_CID_EXPOSURE and > V4L2_CID_ANALOGUE_GAIN). > > It's quite some work, but there shouldn't be anything too complex. If > you're interested in giving this a go I would be glad to help. > > > frame duration is static. Obviously, if the frame duration limit is > > dynamic after start() is called, then it would make sense to also have > > vblank recalculated afterwards. Under my assumption of a static frame > > duration, I guess it would even make sense to put it "before" or outside > > of the IPA-specific ph::start(), as it is just related to the camera > > sensor, and independent of the IPA - but I guess start() is the first > > call to libcamera where the frame durations are actually known. > > > > > 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. > > > > Yes, I think so too. This needs to be implemented per-sensor in the > > helper I guess. I skimmed a little bit through the camera sensor drivers > > and it looks like most of the drivers adapt the v4l2 exposure limits as > > soon as vblank is changed (except e.g. imx258 or imx415). So I guess at > > least it seems to be quite reliable. > > > > So IMHO, for the "given frame duration limit" case, I guess it just > > boils down to the question if the limits can change after calling > > start(). For other use cases, like reducing the frame rate to increase > > exposure when in saturation (or similar), your suggestion might fit > > better. What do you think?
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@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@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@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@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@skidata.com> >> -- Best regards, Mikhail Rudenko
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"; + } + + /* + * 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"; + } + /* 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)