| Message ID | 20210216085342.1012717-4-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On Tue, Feb 16, 2021 at 08:53:40AM +0000, Naushir Patuck wrote: > On DelayedControls::reset(), the values retrieved from the sensor device > were added to the queues with the updated flag set to true. This would > cause the helper to write out the value to the device again on the first > DelayedControls::applyControls() call. This is unnecessary, as the > controls written are identical to what is stored in the device driver. > > Fix this by explicitly setting the update flag to false in > DelayedControls::reset() when adding the controls to the queue. > > Additionally, use the Info() constructor when adding items to the queue > for consistency. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay") > --- > include/libcamera/internal/delayed_controls.h | 4 ++-- > src/libcamera/delayed_controls.cpp | 14 ++++++++------ > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h > index 564d9f2e2440..2a6a912bde10 100644 > --- a/include/libcamera/internal/delayed_controls.h > +++ b/include/libcamera/internal/delayed_controls.h > @@ -43,8 +43,8 @@ private: > { > } > > - Info(const ControlValue &v) > - : ControlValue(v), updated(true) > + Info(const ControlValue &v, bool updated_ = true) > + : ControlValue(v), updated(updated_) > { > } > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > index 3ed1dfebd035..21dc3e164fe9 100644 > --- a/src/libcamera/delayed_controls.cpp > +++ b/src/libcamera/delayed_controls.cpp > @@ -111,7 +111,11 @@ void DelayedControls::reset() > values_.clear(); > for (const auto &ctrl : controls) { > const ControlId *id = device_->controls().idmap().at(ctrl.first); > - values_[id][0] = Info(ctrl.second); > + /* > + * Do not mark this control value as updated, it does not need > + * to be written to to device on startup. > + */ > + values_[id][0] = Info(ctrl.second, false); > } > } > > @@ -126,11 +130,10 @@ void DelayedControls::reset() > */ > bool DelayedControls::push(const ControlList &controls) > { > - /* Copy state from previous frame. */ > + /* Copy state from previous frame and clear the updated flag */ > for (auto &ctrl : values_) { > Info &info = ctrl.second[queueCount_]; > - info = values_[ctrl.first][queueCount_ - 1]; > - info.updated = false; > + info = Info(values_[ctrl.first][queueCount_ - 1], false); Isn't values_[ctrl.first][queueCount_ - 1] an Info instance? I didn't think implicit copy constructor worked with the extra argument. The rest looks good to me. Paul > } > > /* Update with new controls. */ > @@ -150,8 +153,7 @@ bool DelayedControls::push(const ControlList &controls) > > Info &info = values_[id][queueCount_]; > > - info = control.second; > - info.updated = true; > + info = Info(control.second); > > LOG(DelayedControls, Debug) > << "Queuing " << id->name() > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, Thank you for your review feedback. On Mon, 1 Mar 2021 at 09:25, <paul.elder@ideasonboard.com> wrote: > Hi Naush, > > On Tue, Feb 16, 2021 at 08:53:40AM +0000, Naushir Patuck wrote: > > On DelayedControls::reset(), the values retrieved from the sensor device > > were added to the queues with the updated flag set to true. This would > > cause the helper to write out the value to the device again on the first > > DelayedControls::applyControls() call. This is unnecessary, as the > > controls written are identical to what is stored in the device driver. > > > > Fix this by explicitly setting the update flag to false in > > DelayedControls::reset() when adding the controls to the queue. > > > > Additionally, use the Info() constructor when adding items to the queue > > for consistency. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for > controls that apply with a delay") > > --- > > include/libcamera/internal/delayed_controls.h | 4 ++-- > > src/libcamera/delayed_controls.cpp | 14 ++++++++------ > > 2 files changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/include/libcamera/internal/delayed_controls.h > b/include/libcamera/internal/delayed_controls.h > > index 564d9f2e2440..2a6a912bde10 100644 > > --- a/include/libcamera/internal/delayed_controls.h > > +++ b/include/libcamera/internal/delayed_controls.h > > @@ -43,8 +43,8 @@ private: > > { > > } > > > > - Info(const ControlValue &v) > > - : ControlValue(v), updated(true) > > + Info(const ControlValue &v, bool updated_ = true) > > + : ControlValue(v), updated(updated_) > > { > > } > > > > diff --git a/src/libcamera/delayed_controls.cpp > b/src/libcamera/delayed_controls.cpp > > index 3ed1dfebd035..21dc3e164fe9 100644 > > --- a/src/libcamera/delayed_controls.cpp > > +++ b/src/libcamera/delayed_controls.cpp > > @@ -111,7 +111,11 @@ void DelayedControls::reset() > > values_.clear(); > > for (const auto &ctrl : controls) { > > const ControlId *id = > device_->controls().idmap().at(ctrl.first); > > - values_[id][0] = Info(ctrl.second); > > + /* > > + * Do not mark this control value as updated, it does not > need > > + * to be written to to device on startup. > > + */ > > + values_[id][0] = Info(ctrl.second, false); > > } > > } > > > > @@ -126,11 +130,10 @@ void DelayedControls::reset() > > */ > > bool DelayedControls::push(const ControlList &controls) > > { > > - /* Copy state from previous frame. */ > > + /* Copy state from previous frame and clear the updated flag */ > > for (auto &ctrl : values_) { > > Info &info = ctrl.second[queueCount_]; > > - info = values_[ctrl.first][queueCount_ - 1]; > > - info.updated = false; > > + info = Info(values_[ctrl.first][queueCount_ - 1], false); > > Isn't values_[ctrl.first][queueCount_ - 1] an Info instance? I didn't > think implicit copy constructor worked with the extra argument. > It works as expected, but does seem confusing. I will update the code above to be more explicit. Thanks, Naush > > The rest looks good to me. > > > Paul > > > } > > > > /* Update with new controls. */ > > @@ -150,8 +153,7 @@ bool DelayedControls::push(const ControlList > &controls) > > > > Info &info = values_[id][queueCount_]; > > > > - info = control.second; > > - info.updated = true; > > + info = Info(control.second); > > > > LOG(DelayedControls, Debug) > > << "Queuing " << id->name() > > -- > > 2.25.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h index 564d9f2e2440..2a6a912bde10 100644 --- a/include/libcamera/internal/delayed_controls.h +++ b/include/libcamera/internal/delayed_controls.h @@ -43,8 +43,8 @@ private: { } - Info(const ControlValue &v) - : ControlValue(v), updated(true) + Info(const ControlValue &v, bool updated_ = true) + : ControlValue(v), updated(updated_) { } diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp index 3ed1dfebd035..21dc3e164fe9 100644 --- a/src/libcamera/delayed_controls.cpp +++ b/src/libcamera/delayed_controls.cpp @@ -111,7 +111,11 @@ void DelayedControls::reset() values_.clear(); for (const auto &ctrl : controls) { const ControlId *id = device_->controls().idmap().at(ctrl.first); - values_[id][0] = Info(ctrl.second); + /* + * Do not mark this control value as updated, it does not need + * to be written to to device on startup. + */ + values_[id][0] = Info(ctrl.second, false); } } @@ -126,11 +130,10 @@ void DelayedControls::reset() */ bool DelayedControls::push(const ControlList &controls) { - /* Copy state from previous frame. */ + /* Copy state from previous frame and clear the updated flag */ for (auto &ctrl : values_) { Info &info = ctrl.second[queueCount_]; - info = values_[ctrl.first][queueCount_ - 1]; - info.updated = false; + info = Info(values_[ctrl.first][queueCount_ - 1], false); } /* Update with new controls. */ @@ -150,8 +153,7 @@ bool DelayedControls::push(const ControlList &controls) Info &info = values_[id][queueCount_]; - info = control.second; - info.updated = true; + info = Info(control.second); LOG(DelayedControls, Debug) << "Queuing " << id->name()
On DelayedControls::reset(), the values retrieved from the sensor device were added to the queues with the updated flag set to true. This would cause the helper to write out the value to the device again on the first DelayedControls::applyControls() call. This is unnecessary, as the controls written are identical to what is stored in the device driver. Fix this by explicitly setting the update flag to false in DelayedControls::reset() when adding the controls to the queue. Additionally, use the Info() constructor when adding items to the queue for consistency. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay") --- include/libcamera/internal/delayed_controls.h | 4 ++-- src/libcamera/delayed_controls.cpp | 14 ++++++++------ 2 files changed, 10 insertions(+), 8 deletions(-)