| Message ID | 20210128091050.881815-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On Thu, Jan 28, 2021 at 09:10:50AM +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> > --- > 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; Won't nextFrame() set updated to false ? Anyway, this was here already and it's really minor. Feel free to leave it as it is. The patch looks *unfortunately* reasonable. And I say unfortunately as you're really working around a limitation of the kernel API, Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > 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_; > }; > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for your review feedback. On Fri, 29 Jan 2021 at 10:08, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Naush, > > On Thu, Jan 28, 2021 at 09:10:50AM +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> > > --- > > 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; > > Won't nextFrame() set updated to false ? > Anyway, this was here already and it's really minor. Feel free to > leave it as it is. > Slightly different - the above line is clearing the updated flag for the current control index. In nextFrame(), we are clearing the update fag for the next-control-to-be-written index. I do get your point that you only need one of these statements though :-) I'll leave it as it is, on account of this being deprecated soon, and it is slightly unrelated to this patch subject. > > The patch looks *unfortunately* reasonable. And I say unfortunately as > you're really working around a limitation of the kernel API, > Indeed. It does fix the problem, but in a bit of an ugly way :-( Regards, Naush > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > > 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_; > > }; > > -- > > 2.25.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
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_; };
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> --- 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(-)