| Message ID | 20210129111616.1047483-6-naush@raspberrypi.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Fri, Jan 29, 2021 at 11:16:16AM +0000, Naushir Patuck wrote: > If an exposure time change adjusts the vblanking limits, and we write > both VBLANK and EXPOSURE controls as a set, the latter may fail if the > value is outside of the limits calculated by the old VBLANK value. This > is a limitation in V4L2 and cannot be fixed by writing VBLANK before > EXPOSURE. > > The workaround here is to have the StaggeredCtrl mark the > VBLANK control as "priority write", which then write VBLANK separately > from (and ahead of) any other controls. This way, the sensor driver will > update the EXPOSURE control with new limits before the new values is > presented, and will thus be seen as valid. > > In addition to this, the following changes have also been made to > the module: > > - The initializer list passed into init() now uses a structure type > instead of a std::pair. > - Use unsigned int to store control delays to avoid unnecessary casts. > > StaggeredCtrl is due to be deprecated and replaced by DelayedCtrl, so > this change serves more a working proof-of-concept on the workaround, > and not much care has been taken to provide a nice new API for applying > this "priority write" flag to the control. A similar workaround must be > available to DelayedCtrl eventually. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> I'll drop this patch for now, as the StaggeredCtrl implementation has been replaced. > --- > src/ipa/raspberrypi/raspberrypi.cpp | 5 ++- > .../pipeline/raspberrypi/raspberrypi.cpp | 12 ++++-- > .../pipeline/raspberrypi/staggered_ctrl.cpp | 41 +++++++++++++------ > .../pipeline/raspberrypi/staggered_ctrl.h | 17 ++++++-- > 4 files changed, 54 insertions(+), 21 deletions(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 8c0e699184f6..2ad7b7dabb3e 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -1038,8 +1038,9 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > > /* > * Due to the behavior of V4L2, the current value of VBLANK could clip the > - * exposure time without us knowing. The next time though this function should > - * clip exposure correctly. > + * exposure time without us knowing. We get around this by ensuring the > + * staggered write component submits VBLANK separately from, and before the > + * EXPOSURE control. > */ > ctrls.set(V4L2_CID_VBLANK, vblanking); > ctrls.set(V4L2_CID_EXPOSURE, exposureLines); > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 524cc960dd37..2118f2e72486 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1229,12 +1229,18 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > /* > * Setup our staggered control writer with the sensor default > * gain and exposure delays. > + * > + * VBLANK must be flagged as "priority write" to allow it to > + * be set ahead of (and separate from) all other controls that > + * are batched together. This is needed so that any update to the > + * EXPOSURE control will be validated based on the new VBLANK > + * control value. > */ > if (!staggeredCtrl_) { > staggeredCtrl_.init(unicam_[Unicam::Image].dev(), > - { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] }, > - { V4L2_CID_EXPOSURE, result.data[resultIdx++] }, > - { V4L2_CID_VBLANK, result.data[resultIdx++] } }); > + { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++], false }, > + { V4L2_CID_EXPOSURE, result.data[resultIdx++], false }, > + { V4L2_CID_VBLANK, result.data[resultIdx++], true } }); > sensorMetadata_ = result.data[resultIdx++]; > } > } > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > index 62605c0fceee..498cd65b4cb6 100644 > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > @@ -22,21 +22,23 @@ LOG_DEFINE_CATEGORY(RPI_S_W) > namespace RPi { > > void StaggeredCtrl::init(V4L2VideoDevice *dev, > - std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList) > + std::initializer_list<CtrlInitParams> ctrlList) > { > std::lock_guard<std::mutex> lock(lock_); > > dev_ = dev; > - delay_ = delayList; > + ctrlParams_.clear(); > ctrl_.clear(); > > - /* Find the largest delay across all controls. */ > maxDelay_ = 0; > - for (auto const &p : delay_) { > + for (auto const &c : ctrlList) { > LOG(RPI_S_W, Info) << "Init ctrl " > - << utils::hex(p.first) << " with delay " > - << static_cast<int>(p.second); > - maxDelay_ = std::max(maxDelay_, p.second); > + << utils::hex(c.id) << " with delay " > + << static_cast<int>(c.delay); > + > + ctrlParams_[c.id] = { c.delay, c.priorityWrite }; > + /* Find the largest delay across all controls. */ > + maxDelay_ = std::max(maxDelay_, c.delay); > } > > init_ = true; > @@ -67,7 +69,7 @@ bool StaggeredCtrl::set(uint32_t ctrl, int32_t value) > std::lock_guard<std::mutex> lock(lock_); > > /* Can we find this ctrl as one that is registered? */ > - if (delay_.find(ctrl) == delay_.end()) > + if (ctrlParams_.find(ctrl) == ctrlParams_.end()) > return false; > > ctrl_[ctrl][setCount_].value = value; > @@ -82,7 +84,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t> > > for (auto const &p : ctrlList) { > /* Can we find this ctrl? */ > - if (delay_.find(p.first) == delay_.end()) > + if (ctrlParams_.find(p.first) == ctrlParams_.end()) > return false; > > ctrl_[p.first][setCount_] = CtrlInfo(p.second); > @@ -97,7 +99,7 @@ bool StaggeredCtrl::set(const ControlList &controls) > > for (auto const &p : controls) { > /* Can we find this ctrl? */ > - if (delay_.find(p.first) == delay_.end()) > + if (ctrlParams_.find(p.first) == ctrlParams_.end()) > return false; > > ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>()); > @@ -117,12 +119,25 @@ int StaggeredCtrl::write() > ControlList controls(dev_->controls()); > > for (auto &p : ctrl_) { > - int delayDiff = maxDelay_ - delay_[p.first]; > + int delayDiff = maxDelay_ - ctrlParams_[p.first].delay; > int index = std::max<int>(0, setCount_ - delayDiff); > > if (p.second[index].updated) { > - /* We need to write this value out. */ > - controls.set(p.first, p.second[index].value); > + if (ctrlParams_[p.first].priorityWrite) { > + /* > + * This control must be written now, it could > + * affect validity of the other controls. > + */ > + ControlList immediate(dev_->controls()); > + immediate.set(p.first, p.second[index].value); > + dev_->setControls(&immediate); > + } else { > + /* > + * Batch up the list of controls and write them > + * at the end of the function. > + */ > + controls.set(p.first, p.second[index].value); > + } > p.second[index].updated = false; > LOG(RPI_S_W, Debug) << "Writing ctrl " > << utils::hex(p.first) << " to " > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > index 382fa31a6853..637629c0d9a8 100644 > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > @@ -23,6 +23,12 @@ namespace RPi { > class StaggeredCtrl > { > public: > + struct CtrlInitParams { > + unsigned int id; > + unsigned int delay; > + bool priorityWrite; > + }; > + > StaggeredCtrl() > : init_(false), setCount_(0), getCount_(0), maxDelay_(0) > { > @@ -34,7 +40,7 @@ public: > } > > void init(V4L2VideoDevice *dev, > - std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList); > + std::initializer_list<CtrlInitParams> ctrlList); > void reset(); > > void get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0); > @@ -79,12 +85,17 @@ private: > } > }; > > + struct CtrlParams { > + unsigned int delay; > + bool priorityWrite; > + }; > + > bool init_; > uint32_t setCount_; > uint32_t getCount_; > - uint8_t maxDelay_; > + unsigned int maxDelay_; > V4L2VideoDevice *dev_; > - std::unordered_map<uint32_t, uint8_t> delay_; > + std::unordered_map<uint32_t, CtrlParams> ctrlParams_; > std::unordered_map<uint32_t, CircularArray> ctrl_; > std::mutex lock_; > };
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 8c0e699184f6..2ad7b7dabb3e 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -1038,8 +1038,9 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) /* * Due to the behavior of V4L2, the current value of VBLANK could clip the - * exposure time without us knowing. The next time though this function should - * clip exposure correctly. + * exposure time without us knowing. We get around this by ensuring the + * staggered write component submits VBLANK separately from, and before the + * EXPOSURE control. */ ctrls.set(V4L2_CID_VBLANK, vblanking); ctrls.set(V4L2_CID_EXPOSURE, exposureLines); diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 524cc960dd37..2118f2e72486 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1229,12 +1229,18 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) /* * Setup our staggered control writer with the sensor default * gain and exposure delays. + * + * VBLANK must be flagged as "priority write" to allow it to + * be set ahead of (and separate from) all other controls that + * are batched together. This is needed so that any update to the + * EXPOSURE control will be validated based on the new VBLANK + * control value. */ if (!staggeredCtrl_) { staggeredCtrl_.init(unicam_[Unicam::Image].dev(), - { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] }, - { V4L2_CID_EXPOSURE, result.data[resultIdx++] }, - { V4L2_CID_VBLANK, result.data[resultIdx++] } }); + { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++], false }, + { V4L2_CID_EXPOSURE, result.data[resultIdx++], false }, + { V4L2_CID_VBLANK, result.data[resultIdx++], true } }); sensorMetadata_ = result.data[resultIdx++]; } } diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp index 62605c0fceee..498cd65b4cb6 100644 --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp @@ -22,21 +22,23 @@ LOG_DEFINE_CATEGORY(RPI_S_W) namespace RPi { void StaggeredCtrl::init(V4L2VideoDevice *dev, - std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList) + std::initializer_list<CtrlInitParams> ctrlList) { std::lock_guard<std::mutex> lock(lock_); dev_ = dev; - delay_ = delayList; + ctrlParams_.clear(); ctrl_.clear(); - /* Find the largest delay across all controls. */ maxDelay_ = 0; - for (auto const &p : delay_) { + for (auto const &c : ctrlList) { LOG(RPI_S_W, Info) << "Init ctrl " - << utils::hex(p.first) << " with delay " - << static_cast<int>(p.second); - maxDelay_ = std::max(maxDelay_, p.second); + << utils::hex(c.id) << " with delay " + << static_cast<int>(c.delay); + + ctrlParams_[c.id] = { c.delay, c.priorityWrite }; + /* Find the largest delay across all controls. */ + maxDelay_ = std::max(maxDelay_, c.delay); } init_ = true; @@ -67,7 +69,7 @@ bool StaggeredCtrl::set(uint32_t ctrl, int32_t value) std::lock_guard<std::mutex> lock(lock_); /* Can we find this ctrl as one that is registered? */ - if (delay_.find(ctrl) == delay_.end()) + if (ctrlParams_.find(ctrl) == ctrlParams_.end()) return false; ctrl_[ctrl][setCount_].value = value; @@ -82,7 +84,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t> for (auto const &p : ctrlList) { /* Can we find this ctrl? */ - if (delay_.find(p.first) == delay_.end()) + if (ctrlParams_.find(p.first) == ctrlParams_.end()) return false; ctrl_[p.first][setCount_] = CtrlInfo(p.second); @@ -97,7 +99,7 @@ bool StaggeredCtrl::set(const ControlList &controls) for (auto const &p : controls) { /* Can we find this ctrl? */ - if (delay_.find(p.first) == delay_.end()) + if (ctrlParams_.find(p.first) == ctrlParams_.end()) return false; ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>()); @@ -117,12 +119,25 @@ int StaggeredCtrl::write() ControlList controls(dev_->controls()); for (auto &p : ctrl_) { - int delayDiff = maxDelay_ - delay_[p.first]; + int delayDiff = maxDelay_ - ctrlParams_[p.first].delay; int index = std::max<int>(0, setCount_ - delayDiff); if (p.second[index].updated) { - /* We need to write this value out. */ - controls.set(p.first, p.second[index].value); + if (ctrlParams_[p.first].priorityWrite) { + /* + * This control must be written now, it could + * affect validity of the other controls. + */ + ControlList immediate(dev_->controls()); + immediate.set(p.first, p.second[index].value); + dev_->setControls(&immediate); + } else { + /* + * Batch up the list of controls and write them + * at the end of the function. + */ + controls.set(p.first, p.second[index].value); + } p.second[index].updated = false; LOG(RPI_S_W, Debug) << "Writing ctrl " << utils::hex(p.first) << " to " diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h index 382fa31a6853..637629c0d9a8 100644 --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h @@ -23,6 +23,12 @@ namespace RPi { class StaggeredCtrl { public: + struct CtrlInitParams { + unsigned int id; + unsigned int delay; + bool priorityWrite; + }; + StaggeredCtrl() : init_(false), setCount_(0), getCount_(0), maxDelay_(0) { @@ -34,7 +40,7 @@ public: } void init(V4L2VideoDevice *dev, - std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList); + std::initializer_list<CtrlInitParams> ctrlList); void reset(); void get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0); @@ -79,12 +85,17 @@ private: } }; + struct CtrlParams { + unsigned int delay; + bool priorityWrite; + }; + bool init_; uint32_t setCount_; uint32_t getCount_; - uint8_t maxDelay_; + unsigned int maxDelay_; V4L2VideoDevice *dev_; - std::unordered_map<uint32_t, uint8_t> delay_; + std::unordered_map<uint32_t, CtrlParams> ctrlParams_; std::unordered_map<uint32_t, CircularArray> ctrl_; std::mutex lock_; };