[v1,1/3] pipeline: rpi: Simplify delayed controls
diff mbox series

Message ID 20260324151714.3345-2-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Atomic control lists on Raspberry Pi
Related show

Commit Message

David Plowman March 24, 2026, 2:32 p.m. UTC 
The queueCount field in the DelayedControls is actually
redundant. Remove it.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/rpi/common/delayed_controls.cpp  | 22 +++++++++----------
 .../pipeline/rpi/common/delayed_controls.h    |  1 -
 2 files changed, 10 insertions(+), 13 deletions(-)

Comments

Naushir Patuck March 26, 2026, 9:01 a.m. UTC | #1 
Hi David,

Thank you for the patch.

On Tue, 24 Mar 2026 at 15:17, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> The queueCount field in the DelayedControls is actually
> redundant. Remove it.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  .../pipeline/rpi/common/delayed_controls.cpp  | 22 +++++++++----------
>  .../pipeline/rpi/common/delayed_controls.h    |  1 -
>  2 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> index 19c71946..c06bd784 100644
> --- a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> +++ b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> @@ -119,7 +119,6 @@ DelayedControls::DelayedControls(V4L2Device *device,
>   */
>  void DelayedControls::reset(unsigned int cookie)
>  {
> -       queueCount_ = 1;
>         writeCount_ = 0;
>         cookies_[0] = cookie;
>
> @@ -155,8 +154,8 @@ bool DelayedControls::push(const ControlList &controls, const unsigned int cooki
>  {
>         /* Copy state from previous frame. */
>         for (auto &ctrl : values_) {
> -               Info &info = ctrl.second[queueCount_];
> -               info = values_[ctrl.first][queueCount_ - 1];
> +               Info &info = ctrl.second[writeCount_];
> +               info = values_[ctrl.first][writeCount_ - 1];
>                 info.updated = false;
>         }

It's been a while since I've looked into this bit of code, but I'm
fairly confident that the logic is correct.

I had to stop at the above change since there is a possibility that
writeCount_ == 0 and we index into -1.  This should never happen as
the sequencing within our PH always ensures applyControls() will be
called on the first frame.

Just to keep me happy, can we consider adding an if (writeCount_ > 0)
guard around the for loop above?  With this, it would be entirely
valid for push() to be called before applyControls(), even though we
would not do it practically.  Perhaps a log message on the else branch
might also be useful?

Regards,
Naush

>
> @@ -175,18 +174,17 @@ bool DelayedControls::push(const ControlList &controls, const unsigned int cooki
>                 if (controlParams_.find(id) == controlParams_.end())
>                         return false;
>
> -               Info &info = values_[id][queueCount_];
> +               Info &info = values_[id][writeCount_];
>
>                 info = Info(control.second);
>
>                 LOG(RPiDelayedControls, Debug)
>                         << "Queuing " << id->name()
>                         << " to " << info.toString()
> -                       << " at index " << queueCount_;
> +                       << " at index " << writeCount_;
>         }
>
> -       cookies_[queueCount_] = cookie;
> -       queueCount_++;
> +       cookies_[writeCount_] = cookie;
>
>         return true;
>  }
> @@ -277,12 +275,12 @@ void DelayedControls::applyControls(uint32_t sequence)
>                 }
>         }
>
> -       writeCount_ = sequence + 1;
> -
> -       while (writeCount_ > queueCount_) {
> +       while (writeCount_ < sequence + 1) {
> +               writeCount_++;
>                 LOG(RPiDelayedControls, Debug)
> -                       << "Queue is empty, auto queue no-op.";
> -               push({}, cookies_[queueCount_ - 1]);
> +                       << "Pushing noop with for index " << writeCount_
> +                       << " with cookie " << cookies_[writeCount_ - 1];
> +               push({}, cookies_[writeCount_ - 1]);
>         }
>
>         device_->setControls(&out);
> diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.h b/src/libcamera/pipeline/rpi/common/delayed_controls.h
> index 487b0057..48ad5a0f 100644
> --- a/src/libcamera/pipeline/rpi/common/delayed_controls.h
> +++ b/src/libcamera/pipeline/rpi/common/delayed_controls.h
> @@ -76,7 +76,6 @@ private:
>         std::unordered_map<const ControlId *, ControlParams> controlParams_;
>         unsigned int maxDelay_;
>
> -       uint32_t queueCount_;
>         uint32_t writeCount_;
>         std::unordered_map<const ControlId *, RingBuffer<Info>> values_;
>         RingBuffer<unsigned int> cookies_;
> --
> 2.47.3
>
David Plowman March 27, 2026, 11:37 a.m. UTC | #2 
Hi Naush

Thanks for the review.

On Thu, 26 Mar 2026 at 09:02, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Tue, 24 Mar 2026 at 15:17, David Plowman
> <david.plowman@raspberrypi.com> wrote:
> >
> > The queueCount field in the DelayedControls is actually
> > redundant. Remove it.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  .../pipeline/rpi/common/delayed_controls.cpp  | 22 +++++++++----------
> >  .../pipeline/rpi/common/delayed_controls.h    |  1 -
> >  2 files changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> > index 19c71946..c06bd784 100644
> > --- a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> > @@ -119,7 +119,6 @@ DelayedControls::DelayedControls(V4L2Device *device,
> >   */
> >  void DelayedControls::reset(unsigned int cookie)
> >  {
> > -       queueCount_ = 1;
> >         writeCount_ = 0;
> >         cookies_[0] = cookie;
> >
> > @@ -155,8 +154,8 @@ bool DelayedControls::push(const ControlList &controls, const unsigned int cooki
> >  {
> >         /* Copy state from previous frame. */
> >         for (auto &ctrl : values_) {
> > -               Info &info = ctrl.second[queueCount_];
> > -               info = values_[ctrl.first][queueCount_ - 1];
> > +               Info &info = ctrl.second[writeCount_];
> > +               info = values_[ctrl.first][writeCount_ - 1];
> >                 info.updated = false;
> >         }
>
> It's been a while since I've looked into this bit of code, but I'm
> fairly confident that the logic is correct.
>
> I had to stop at the above change since there is a possibility that
> writeCount_ == 0 and we index into -1.  This should never happen as
> the sequencing within our PH always ensures applyControls() will be
> called on the first frame.
>
> Just to keep me happy, can we consider adding an if (writeCount_ > 0)
> guard around the for loop above?  With this, it would be entirely
> valid for push() to be called before applyControls(), even though we
> would not do it practically.  Perhaps a log message on the else branch
> might also be useful?
>
> Regards,
> Naush

Yep, I'll add that.

Thanks
David

>
> >
> > @@ -175,18 +174,17 @@ bool DelayedControls::push(const ControlList &controls, const unsigned int cooki
> >                 if (controlParams_.find(id) == controlParams_.end())
> >                         return false;
> >
> > -               Info &info = values_[id][queueCount_];
> > +               Info &info = values_[id][writeCount_];
> >
> >                 info = Info(control.second);
> >
> >                 LOG(RPiDelayedControls, Debug)
> >                         << "Queuing " << id->name()
> >                         << " to " << info.toString()
> > -                       << " at index " << queueCount_;
> > +                       << " at index " << writeCount_;
> >         }
> >
> > -       cookies_[queueCount_] = cookie;
> > -       queueCount_++;
> > +       cookies_[writeCount_] = cookie;
> >
> >         return true;
> >  }
> > @@ -277,12 +275,12 @@ void DelayedControls::applyControls(uint32_t sequence)
> >                 }
> >         }
> >
> > -       writeCount_ = sequence + 1;
> > -
> > -       while (writeCount_ > queueCount_) {
> > +       while (writeCount_ < sequence + 1) {
> > +               writeCount_++;
> >                 LOG(RPiDelayedControls, Debug)
> > -                       << "Queue is empty, auto queue no-op.";
> > -               push({}, cookies_[queueCount_ - 1]);
> > +                       << "Pushing noop with for index " << writeCount_
> > +                       << " with cookie " << cookies_[writeCount_ - 1];
> > +               push({}, cookies_[writeCount_ - 1]);
> >         }
> >
> >         device_->setControls(&out);
> > diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.h b/src/libcamera/pipeline/rpi/common/delayed_controls.h
> > index 487b0057..48ad5a0f 100644
> > --- a/src/libcamera/pipeline/rpi/common/delayed_controls.h
> > +++ b/src/libcamera/pipeline/rpi/common/delayed_controls.h
> > @@ -76,7 +76,6 @@ private:
> >         std::unordered_map<const ControlId *, ControlParams> controlParams_;
> >         unsigned int maxDelay_;
> >
> > -       uint32_t queueCount_;
> >         uint32_t writeCount_;
> >         std::unordered_map<const ControlId *, RingBuffer<Info>> values_;
> >         RingBuffer<unsigned int> cookies_;
> > --
> > 2.47.3
> >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
index 19c71946..c06bd784 100644
--- a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
+++ b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
@@ -119,7 +119,6 @@  DelayedControls::DelayedControls(V4L2Device *device,
  */
 void DelayedControls::reset(unsigned int cookie)
 {
-	queueCount_ = 1;
 	writeCount_ = 0;
 	cookies_[0] = cookie;
 
@@ -155,8 +154,8 @@  bool DelayedControls::push(const ControlList &controls, const unsigned int cooki
 {
 	/* Copy state from previous frame. */
 	for (auto &ctrl : values_) {
-		Info &info = ctrl.second[queueCount_];
-		info = values_[ctrl.first][queueCount_ - 1];
+		Info &info = ctrl.second[writeCount_];
+		info = values_[ctrl.first][writeCount_ - 1];
 		info.updated = false;
 	}
 
@@ -175,18 +174,17 @@  bool DelayedControls::push(const ControlList &controls, const unsigned int cooki
 		if (controlParams_.find(id) == controlParams_.end())
 			return false;
 
-		Info &info = values_[id][queueCount_];
+		Info &info = values_[id][writeCount_];
 
 		info = Info(control.second);
 
 		LOG(RPiDelayedControls, Debug)
 			<< "Queuing " << id->name()
 			<< " to " << info.toString()
-			<< " at index " << queueCount_;
+			<< " at index " << writeCount_;
 	}
 
-	cookies_[queueCount_] = cookie;
-	queueCount_++;
+	cookies_[writeCount_] = cookie;
 
 	return true;
 }
@@ -277,12 +275,12 @@  void DelayedControls::applyControls(uint32_t sequence)
 		}
 	}
 
-	writeCount_ = sequence + 1;
-
-	while (writeCount_ > queueCount_) {
+	while (writeCount_ < sequence + 1) {
+		writeCount_++;
 		LOG(RPiDelayedControls, Debug)
-			<< "Queue is empty, auto queue no-op.";
-		push({}, cookies_[queueCount_ - 1]);
+			<< "Pushing noop with for index " << writeCount_
+			<< " with cookie " << cookies_[writeCount_ - 1];
+		push({}, cookies_[writeCount_ - 1]);
 	}
 
 	device_->setControls(&out);
diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.h b/src/libcamera/pipeline/rpi/common/delayed_controls.h
index 487b0057..48ad5a0f 100644
--- a/src/libcamera/pipeline/rpi/common/delayed_controls.h
+++ b/src/libcamera/pipeline/rpi/common/delayed_controls.h
@@ -76,7 +76,6 @@  private:
 	std::unordered_map<const ControlId *, ControlParams> controlParams_;
 	unsigned int maxDelay_;
 
-	uint32_t queueCount_;
 	uint32_t writeCount_;
 	std::unordered_map<const ControlId *, RingBuffer<Info>> values_;
 	RingBuffer<unsigned int> cookies_;