[RFC,1/7] libcamera: delayed_controls: Add push() function that accepts a sequence number

22441 diff mbox series

The push function is assymetric to the get() and applyControls()
function in that it doesn't allow to specify a frame number. This leads
to the unfortunate situation that it is very difficult to detect if
anything goes out of sync. Add a version of the push() function that
takes a sequence paramater and warns when the sequence provided differs
from the expected sequence. Don't take any further actions for now to
see where issues pop up.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 include/libcamera/internal/delayed_controls.h |  1 +
 src/libcamera/delayed_controls.cpp            | 33 +++++++++++++++++--
 2 files changed, 32 insertions(+), 2 deletions(-)

On Fri, Dec 20, 2024 at 05:26:47PM +0100, Stefan Klug wrote:
> The push function is assymetric to the get() and applyControls()
> function in that it doesn't allow to specify a frame number. This leads
> to the unfortunate situation that it is very difficult to detect if
> anything goes out of sync. Add a version of the push() function that
> takes a sequence paramater and warns when the sequence provided differs
> from the expected sequence. Don't take any further actions for now to
> see where issues pop up.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  include/libcamera/internal/delayed_controls.h |  1 +
>  src/libcamera/delayed_controls.cpp            | 33 +++++++++++++++++--
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> index e8d3014d92cb..f1467fd79814 100644
> --- a/include/libcamera/internal/delayed_controls.h
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -30,6 +30,7 @@ public:
>  	void reset();
>  
>  	bool push(const ControlList &controls);
> +	bool push(uint32_t sequence, const ControlList &controls);
>  	ControlList get(uint32_t sequence);
>  
>  	void applyControls(uint32_t sequence);
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 94d0a575b01b..6c82d5058ae8 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -144,10 +144,39 @@ void DelayedControls::reset()
>   * Push a set of controls to the control queue. This increases the control queue
>   * depth by one.
>   *
> + * \note The usage of this function is discouraged as it does not provide a way
> + * to detect double pushes for the same sequence. Better
> + * DelayedControls::push(uint32_t sequence, const ControlList &controls)
> + * instead.
> + *
>   * \returns true if \a controls are accepted, or false otherwise
>   */
> -bool DelayedControls::push(const ControlList &controls)
> +bool DelayedControls::push(const ControlList &controls) {
> +	LOG(DelayedControls, Debug) << "Deprecated: Push without sequence number";
> +	return push(queueCount_, controls);
> +}
> +
> +/**
> + * \brief Push a set of controls on the queue
> + * \param[in] sequence The sequence number to push for
> + * \param[in] controls List of controls to add to the device queue
> + *
> + * Push a set of controls to the control queue. This increases the control queue
> + * depth by one.
> + *
> + * The \a sequence number is used to do some sanity checks to detect double
> + * pushes for the same sequence (either due to a bug or a request underrun).
> + *
> + * \returns true if \a controls are accepted, or false otherwise
> + */
> +bool DelayedControls::push(uint32_t sequence, const ControlList &controls)
>  {
> +	if(sequence != queueCount_) {
> +		LOG(DelayedControls, Warning)
> +			<< "Double push for sequence " << sequence
> +			<< " current queue index: " << queueCount_;
> +	}
> +
>  	/* Copy state from previous frame. */
>  	for (auto &ctrl : values_) {
>  		Info &info = ctrl.second[queueCount_];
> @@ -276,7 +305,7 @@ void DelayedControls::applyControls(uint32_t sequence)
>  	while (writeCount_ > queueCount_) {
>  		LOG(DelayedControls, Debug)
>  			<< "Queue is empty, auto queue no-op.";
> -		push({});
> +		push(queueCount_, {});
>  	}
>  
>  	device_->setControls(&out);
> -- 
> 2.43.0
>