[libcamera-devel] [PATCH v2 3/5] libcamera: delayed_controls: Remove unneeded write when starting up

[Naushir Patuck]

Hi Paul,

Thank you for your review feedback.

On Mon, 1 Mar 2021 at 09:25, <paul.elder at 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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210301/ca065636/attachment.htm>