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

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

Commit Message

David Plowman March 27, 2026, 2:42 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  | 32 +++++++++++--------
 .../pipeline/rpi/common/delayed_controls.h    |  1 -
 2 files changed, 18 insertions(+), 15 deletions(-)

Comments

Naushir Patuck March 30, 2026, 7:38 a.m. UTC | #1 
Hi David,

Thank you for the update.

On Fri, 27 Mar 2026 at 14:47, 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>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  .../pipeline/rpi/common/delayed_controls.cpp  | 32 +++++++++++--------
>  .../pipeline/rpi/common/delayed_controls.h    |  1 -
>  2 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> index 19c71946..93145d30 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;
>
> @@ -154,10 +153,16 @@ void DelayedControls::reset(unsigned int cookie)
>  bool DelayedControls::push(const ControlList &controls, const unsigned int cookie)
>  {
>         /* Copy state from previous frame. */
> -       for (auto &ctrl : values_) {
> -               Info &info = ctrl.second[queueCount_];
> -               info = values_[ctrl.first][queueCount_ - 1];
> -               info.updated = false;
> +       if (writeCount_ > 0) {
> +               for (auto &ctrl : values_) {
> +                       Info &info = ctrl.second[writeCount_];
> +                       info = values_[ctrl.first][writeCount_ - 1];
> +                       info.updated = false;
> +               }
> +       } else {
> +               /* Although it works, we don't expect this. */
> +               LOG(RPiDelayedControls, Warning)
> +                       << "push called before applyControls";
>         }
>
>         /* Update with new controls. */
> @@ -175,18 +180,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 +281,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
>
Laurent Pinchart April 27, 2026, 12:33 a.m. UTC | #2 
Hi David,

Thank you for the patch.

On Fri, Mar 27, 2026 at 02:42:34PM +0000, David Plowman wrote:
> The queueCount field in the DelayedControls is actually
> redundant. Remove it.

This patch changes the behaviour of the class significantly. Calling
push() twice with applyControls() in-between will not result in both
push() calls writing to the same entry in the ring buffer. The commit
message should explain why this is desired, and why it's not an issue.
The documentation of the push() function should also be updated, and
possibly other pieces of documentation as well.

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  .../pipeline/rpi/common/delayed_controls.cpp  | 32 +++++++++++--------
>  .../pipeline/rpi/common/delayed_controls.h    |  1 -
>  2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> index 19c71946..93145d30 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;
>  
> @@ -154,10 +153,16 @@ void DelayedControls::reset(unsigned int cookie)
>  bool DelayedControls::push(const ControlList &controls, const unsigned int cookie)
>  {
>  	/* Copy state from previous frame. */
> -	for (auto &ctrl : values_) {
> -		Info &info = ctrl.second[queueCount_];
> -		info = values_[ctrl.first][queueCount_ - 1];
> -		info.updated = false;
> +	if (writeCount_ > 0) {
> +		for (auto &ctrl : values_) {
> +			Info &info = ctrl.second[writeCount_];
> +			info = values_[ctrl.first][writeCount_ - 1];
> +			info.updated = false;
> +		}
> +	} else {
> +		/* Although it works, we don't expect this. */
> +		LOG(RPiDelayedControls, Warning)
> +			<< "push called before applyControls";
>  	}
>  
>  	/* Update with new controls. */
> @@ -175,18 +180,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 +281,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_;
David Plowman April 27, 2026, 10:04 a.m. UTC | #3 
Hi Laurent

On Mon, 27 Apr 2026 at 01:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Fri, Mar 27, 2026 at 02:42:34PM +0000, David Plowman wrote:
> > The queueCount field in the DelayedControls is actually
> > redundant. Remove it.
>
> This patch changes the behaviour of the class significantly. Calling
> push() twice with applyControls() in-between will not result in both
> push() calls writing to the same entry in the ring buffer. The commit
> message should explain why this is desired, and why it's not an issue.
> The documentation of the push() function should also be updated, and
> possibly other pieces of documentation as well.

Yes, thanks for the question. The critical thing is that
push/applyControls calls are always interleaved by our PH, so
queueCount and writeCount are basically always the same (or different
by one or something). But I'll add a little more explanation around
that and amend some documentation.

Naush reminds me that there's a bit of history around this code, so
it's actually a slight case of "Back to the Future" for us!

Thanks

David

>
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  .../pipeline/rpi/common/delayed_controls.cpp  | 32 +++++++++++--------
> >  .../pipeline/rpi/common/delayed_controls.h    |  1 -
> >  2 files changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/delayed_controls.cpp b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> > index 19c71946..93145d30 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;
> >
> > @@ -154,10 +153,16 @@ void DelayedControls::reset(unsigned int cookie)
> >  bool DelayedControls::push(const ControlList &controls, const unsigned int cookie)
> >  {
> >       /* Copy state from previous frame. */
> > -     for (auto &ctrl : values_) {
> > -             Info &info = ctrl.second[queueCount_];
> > -             info = values_[ctrl.first][queueCount_ - 1];
> > -             info.updated = false;
> > +     if (writeCount_ > 0) {
> > +             for (auto &ctrl : values_) {
> > +                     Info &info = ctrl.second[writeCount_];
> > +                     info = values_[ctrl.first][writeCount_ - 1];
> > +                     info.updated = false;
> > +             }
> > +     } else {
> > +             /* Although it works, we don't expect this. */
> > +             LOG(RPiDelayedControls, Warning)
> > +                     << "push called before applyControls";
> >       }
> >
> >       /* Update with new controls. */
> > @@ -175,18 +180,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 +281,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_;
>
> --
> Regards,
>
> Laurent Pinchart

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..93145d30 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;
 
@@ -154,10 +153,16 @@  void DelayedControls::reset(unsigned int cookie)
 bool DelayedControls::push(const ControlList &controls, const unsigned int cookie)
 {
 	/* Copy state from previous frame. */
-	for (auto &ctrl : values_) {
-		Info &info = ctrl.second[queueCount_];
-		info = values_[ctrl.first][queueCount_ - 1];
-		info.updated = false;
+	if (writeCount_ > 0) {
+		for (auto &ctrl : values_) {
+			Info &info = ctrl.second[writeCount_];
+			info = values_[ctrl.first][writeCount_ - 1];
+			info.updated = false;
+		}
+	} else {
+		/* Although it works, we don't expect this. */
+		LOG(RPiDelayedControls, Warning)
+			<< "push called before applyControls";
 	}
 
 	/* Update with new controls. */
@@ -175,18 +180,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 +281,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_;