| Message ID | 20260508130804.8238-4-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David On Fri, May 08, 2026 at 01:57:43PM +0100, David Plowman wrote: > When a request is about to be processed, this commit separates > "delayed controls" (camera-related ones that take "a few frames" to > apply) from "immediate controls" (ISP-related controls) that will > happen instantly. > > The immediate controls are held back until the delayed controls that > they were submitted with, have happened. We merge back together the > immediate controls that must happen now, along with the delayed > controls that must be forwarded early, the result being that control > lists submitted with a request happen atomically. We avoid overwriting > the original control list submitted with the request. > > We therefore already have the sequence number of the request whose > controls have just been applied, so we additionally attach this to the > current request as "ControlId" metadata. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> Thanks for addressing the latest comments Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > src/ipa/rpi/common/ipa_base.cpp | 24 ++++++--- > .../pipeline/rpi/common/pipeline_base.cpp | 49 +++++++++++++++++++ > .../pipeline/rpi/common/pipeline_base.h | 9 ++++ > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++-- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +++-- > 5 files changed, 88 insertions(+), 16 deletions(-) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index faa77719..dacafa57 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -431,6 +431,15 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) > rpiMetadata.clear(); > fillDeviceStatus(params.sensorControls, ipaContext); > > + /* > + * When there are controls, it's important that we don't skip running the > + * IPAs, as that can mess with synchronisation. Crucially though, we need > + * to know whether there were controls when this comes back as the > + * _delayed_ metadata, hence why we flag this in the metadata itself. > + */ > + if (!params.requestControls.empty()) > + rpiMetadata.set("ipa.request_controls", true); > + > if (params.buffers.embedded) { > /* > * Pipeline handler has supplied us with an embedded data buffer, > @@ -451,7 +460,7 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) > */ > AgcStatus agcStatus; > bool hdrChange = false; > - RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext]; > + RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext % rpiMetadata_.size()]; > if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus)) { > rpiMetadata.set("agc.delayed_status", agcStatus); > hdrChange = agcStatus.hdr.mode != hdrStatus_.mode; > @@ -464,9 +473,13 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) > */ > helper_->prepare(embeddedBuffer, rpiMetadata); > > + bool delayedRequestControls = false; > + delayedMetadata.get<bool>("ipa.request_controls", delayedRequestControls); > + > /* Allow a 10% margin on the comparison below. */ > Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; > - if (lastRunTimestamp_ && frameCount_ > invalidCount_ && > + if (!delayedRequestControls && params.requestControls.empty() && > + lastRunTimestamp_ && frameCount_ > invalidCount_ && > delta < controllerMinFrameDuration_ * 0.9 && !hdrChange) { > /* > * Ensure we merge the previous frame's metadata with the current > @@ -535,7 +548,7 @@ void IpaBase::processStats(const ProcessParams ¶ms) > ControlList ctrls(sensorCtrls_); > applyAGC(&agcStatus, ctrls); > rpiMetadata.set("agc.status", agcStatus); > - setDelayedControls.emit(ctrls, ipaContext); > + setDelayedControls.emit(ctrls, params.ipaContext); > setCameraTimeoutValue(); > } > > @@ -951,8 +964,6 @@ void IpaBase::applyControls(const ControlList &controls) > > /* The control provides units of microseconds. */ > agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us); > - > - libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>()); > break; > } > > @@ -976,9 +987,6 @@ void IpaBase::applyControls(const ControlList &controls) > break; > > agc->setFixedGain(0, ctrl.second.get<float>()); > - > - libcameraMetadata_.set(controls::AnalogueGain, > - ctrl.second.get<float>()); > break; > } > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index f6ef8a3b..ace38997 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -1528,4 +1528,53 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request > } > } > > +static bool isControlDelayed(unsigned int id) > +{ > + return id == controls::ExposureTime || > + id == controls::AnalogueGain || > + id == controls::FrameDurationLimits || > + id == controls::AeEnable || > + id == controls::ExposureTimeMode || > + id == controls::AnalogueGainMode; > +} > + > +void CameraData::handleControlLists(uint32_t delayContext, ControlList ¶mControls) > +{ > + /* > + * The delayContext is the sequence number after it's gone through the various > + * pipeline delays, so that's what gets reported as the "ControlListSequence" > + * in the metadata, being the sequence number of the request whose ControlList > + * has just been applied. > + */ > + Request *request = requestQueue_.front(); > + request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext); > + > + /* > + * Controls that take effect immediately (typically ISP controls) have to be > + * delayed so as to synchronise with those controls that do get delayed. So we > + * must remove them from the current request, and push them onto a queue so > + * that they can be used later. > + * > + * Note that we are given a separate control list (paramControls) so that > + * we can pass back the controls that really need to happen now, without > + * disturbing the controls that were submitted with the request. > + */ > + ASSERT(paramControls.empty()); > + immediateControls_.push({ request->sequence(), {} }); > + for (const auto &ctrl : request->controls()) { > + if (isControlDelayed(ctrl.first)) > + paramControls.set(ctrl.first, ctrl.second); > + else > + immediateControls_.back().controls.set(ctrl.first, ctrl.second); > + } > + > + /* "Immediate" controls that have become due are now merged back into this request. */ > + while (!immediateControls_.empty() && > + immediateControls_.front().controlListId <= delayContext) { > + paramControls.merge(immediateControls_.front().controls, > + ControlList::MergePolicy::OverwriteExisting); > + immediateControls_.pop(); > + } > +} > + > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h > index 7bfac33e..758155ee 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > @@ -180,10 +180,19 @@ public: > > ClockRecovery wallClockRecovery_; > > + struct ImmediateControlsEntry { > + uint64_t controlListId; > + ControlList controls; > + }; > + std::queue<ImmediateControlsEntry> immediateControls_; > + > protected: > void fillRequestMetadata(const ControlList &bufferControls, > Request *request); > > + void handleControlLists(uint32_t delayContext, > + ControlList ¶mControls); > + > virtual void tryRunPipeline() = 0; > > private: > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > index cc7e32c3..b744c901 100644 > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > @@ -2322,9 +2322,6 @@ void PiSPCameraData::tryRunPipeline() > > fillRequestMetadata(job.sensorControls, request); > > - /* Set our state to say the pipeline is active. */ > - state_ = State::Busy; > - > unsigned int bayerId = cfe_[Cfe::Output0].getBufferId(job.buffers[&cfe_[Cfe::Output0]]); > unsigned int statsId = cfe_[Cfe::Stats].getBufferId(job.buffers[&cfe_[Cfe::Stats]]); > ASSERT(bayerId && statsId); > @@ -2341,7 +2338,13 @@ void PiSPCameraData::tryRunPipeline() > params.ipaContext = requestQueue_.front()->sequence(); > params.delayContext = job.delayContext; > params.sensorControls = std::move(job.sensorControls); > - params.requestControls = request->controls(); > + /* params.requestControls is set by handleControlLists. */ > + > + /* This sorts out synchronisation with ControlLists in earlier requests. */ > + handleControlLists(job.delayContext, params.requestControls); > + > + /* Set our state to say the pipeline is active. */ > + state_ = State::Busy; > > if (sensorMetadata_) { > unsigned int embeddedId = > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index f99cfdbc..3e9a4905 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -939,9 +939,6 @@ void Vc4CameraData::tryRunPipeline() > > fillRequestMetadata(bayerFrame.controls, request); > > - /* Set our state to say the pipeline is active. */ > - state_ = State::Busy; > - > unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer); > > LOG(RPI, Debug) << "Signalling prepareIsp:" > @@ -950,10 +947,16 @@ void Vc4CameraData::tryRunPipeline() > ipa::RPi::PrepareParams params; > params.buffers.bayer = RPi::MaskBayerData | bayer; > params.sensorControls = std::move(bayerFrame.controls); > - params.requestControls = request->controls(); > params.ipaContext = request->sequence(); > params.delayContext = bayerFrame.delayContext; > params.buffers.embedded = 0; > + /* params.requestControls is set by handleControlLists. */ > + > + /* This sorts out synchronisation with ControlLists in earlier requests. */ > + handleControlLists(bayerFrame.delayContext, params.requestControls); > + > + /* Set our state to say the pipeline is active. */ > + state_ = State::Busy; > > if (embeddedBuffer) { > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > -- > 2.47.3 >
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index faa77719..dacafa57 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -431,6 +431,15 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) rpiMetadata.clear(); fillDeviceStatus(params.sensorControls, ipaContext); + /* + * When there are controls, it's important that we don't skip running the + * IPAs, as that can mess with synchronisation. Crucially though, we need + * to know whether there were controls when this comes back as the + * _delayed_ metadata, hence why we flag this in the metadata itself. + */ + if (!params.requestControls.empty()) + rpiMetadata.set("ipa.request_controls", true); + if (params.buffers.embedded) { /* * Pipeline handler has supplied us with an embedded data buffer, @@ -451,7 +460,7 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) */ AgcStatus agcStatus; bool hdrChange = false; - RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext]; + RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext % rpiMetadata_.size()]; if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus)) { rpiMetadata.set("agc.delayed_status", agcStatus); hdrChange = agcStatus.hdr.mode != hdrStatus_.mode; @@ -464,9 +473,13 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) */ helper_->prepare(embeddedBuffer, rpiMetadata); + bool delayedRequestControls = false; + delayedMetadata.get<bool>("ipa.request_controls", delayedRequestControls); + /* Allow a 10% margin on the comparison below. */ Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; - if (lastRunTimestamp_ && frameCount_ > invalidCount_ && + if (!delayedRequestControls && params.requestControls.empty() && + lastRunTimestamp_ && frameCount_ > invalidCount_ && delta < controllerMinFrameDuration_ * 0.9 && !hdrChange) { /* * Ensure we merge the previous frame's metadata with the current @@ -535,7 +548,7 @@ void IpaBase::processStats(const ProcessParams ¶ms) ControlList ctrls(sensorCtrls_); applyAGC(&agcStatus, ctrls); rpiMetadata.set("agc.status", agcStatus); - setDelayedControls.emit(ctrls, ipaContext); + setDelayedControls.emit(ctrls, params.ipaContext); setCameraTimeoutValue(); } @@ -951,8 +964,6 @@ void IpaBase::applyControls(const ControlList &controls) /* The control provides units of microseconds. */ agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us); - - libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>()); break; } @@ -976,9 +987,6 @@ void IpaBase::applyControls(const ControlList &controls) break; agc->setFixedGain(0, ctrl.second.get<float>()); - - libcameraMetadata_.set(controls::AnalogueGain, - ctrl.second.get<float>()); break; } diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index f6ef8a3b..ace38997 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -1528,4 +1528,53 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request } } +static bool isControlDelayed(unsigned int id) +{ + return id == controls::ExposureTime || + id == controls::AnalogueGain || + id == controls::FrameDurationLimits || + id == controls::AeEnable || + id == controls::ExposureTimeMode || + id == controls::AnalogueGainMode; +} + +void CameraData::handleControlLists(uint32_t delayContext, ControlList ¶mControls) +{ + /* + * The delayContext is the sequence number after it's gone through the various + * pipeline delays, so that's what gets reported as the "ControlListSequence" + * in the metadata, being the sequence number of the request whose ControlList + * has just been applied. + */ + Request *request = requestQueue_.front(); + request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext); + + /* + * Controls that take effect immediately (typically ISP controls) have to be + * delayed so as to synchronise with those controls that do get delayed. So we + * must remove them from the current request, and push them onto a queue so + * that they can be used later. + * + * Note that we are given a separate control list (paramControls) so that + * we can pass back the controls that really need to happen now, without + * disturbing the controls that were submitted with the request. + */ + ASSERT(paramControls.empty()); + immediateControls_.push({ request->sequence(), {} }); + for (const auto &ctrl : request->controls()) { + if (isControlDelayed(ctrl.first)) + paramControls.set(ctrl.first, ctrl.second); + else + immediateControls_.back().controls.set(ctrl.first, ctrl.second); + } + + /* "Immediate" controls that have become due are now merged back into this request. */ + while (!immediateControls_.empty() && + immediateControls_.front().controlListId <= delayContext) { + paramControls.merge(immediateControls_.front().controls, + ControlList::MergePolicy::OverwriteExisting); + immediateControls_.pop(); + } +} + } /* namespace libcamera */ diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h index 7bfac33e..758155ee 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h @@ -180,10 +180,19 @@ public: ClockRecovery wallClockRecovery_; + struct ImmediateControlsEntry { + uint64_t controlListId; + ControlList controls; + }; + std::queue<ImmediateControlsEntry> immediateControls_; + protected: void fillRequestMetadata(const ControlList &bufferControls, Request *request); + void handleControlLists(uint32_t delayContext, + ControlList ¶mControls); + virtual void tryRunPipeline() = 0; private: diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp index cc7e32c3..b744c901 100644 --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp @@ -2322,9 +2322,6 @@ void PiSPCameraData::tryRunPipeline() fillRequestMetadata(job.sensorControls, request); - /* Set our state to say the pipeline is active. */ - state_ = State::Busy; - unsigned int bayerId = cfe_[Cfe::Output0].getBufferId(job.buffers[&cfe_[Cfe::Output0]]); unsigned int statsId = cfe_[Cfe::Stats].getBufferId(job.buffers[&cfe_[Cfe::Stats]]); ASSERT(bayerId && statsId); @@ -2341,7 +2338,13 @@ void PiSPCameraData::tryRunPipeline() params.ipaContext = requestQueue_.front()->sequence(); params.delayContext = job.delayContext; params.sensorControls = std::move(job.sensorControls); - params.requestControls = request->controls(); + /* params.requestControls is set by handleControlLists. */ + + /* This sorts out synchronisation with ControlLists in earlier requests. */ + handleControlLists(job.delayContext, params.requestControls); + + /* Set our state to say the pipeline is active. */ + state_ = State::Busy; if (sensorMetadata_) { unsigned int embeddedId = diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp index f99cfdbc..3e9a4905 100644 --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp @@ -939,9 +939,6 @@ void Vc4CameraData::tryRunPipeline() fillRequestMetadata(bayerFrame.controls, request); - /* Set our state to say the pipeline is active. */ - state_ = State::Busy; - unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer); LOG(RPI, Debug) << "Signalling prepareIsp:" @@ -950,10 +947,16 @@ void Vc4CameraData::tryRunPipeline() ipa::RPi::PrepareParams params; params.buffers.bayer = RPi::MaskBayerData | bayer; params.sensorControls = std::move(bayerFrame.controls); - params.requestControls = request->controls(); params.ipaContext = request->sequence(); params.delayContext = bayerFrame.delayContext; params.buffers.embedded = 0; + /* params.requestControls is set by handleControlLists. */ + + /* This sorts out synchronisation with ControlLists in earlier requests. */ + handleControlLists(bayerFrame.delayContext, params.requestControls); + + /* Set our state to say the pipeline is active. */ + state_ = State::Busy; if (embeddedBuffer) { unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);