[v2,2/5] libcamera: pipeline_handler: Allow to limit the number of queued requests

23745 diff mbox series

Add a maxQueuedRequestsDevice constructor parameter to allow pipeline
handler classes to limit the maximum number of requests that get queued
to the device in queueRequestDevice().

The default value is set to an arbitrary number of 32 which is big
enough for all currently known use cases.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- Moved the fix to properly handle the waitingRequests queue when
  stopping the device into this patch as they belong together
- Added documentation for maxQueuedRequestsDevice_
- Fixed an issue with doQueueRequests beeing() called recursively

Changes in v1:
- Used a const member variable to carry the maximum number of requests
- Improved commit message
- Added docs
---
 include/libcamera/internal/pipeline_handler.h |  4 +-
 src/libcamera/pipeline_handler.cpp            | 56 +++++++++++++++----
 2 files changed, 48 insertions(+), 12 deletions(-)

Quoting Stefan Klug (2025-07-07 08:53:36)
> Add a maxQueuedRequestsDevice constructor parameter to allow pipeline
> handler classes to limit the maximum number of requests that get queued
> to the device in queueRequestDevice().
> 
> The default value is set to an arbitrary number of 32 which is big
> enough for all currently known use cases.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Moved the fix to properly handle the waitingRequests queue when
>   stopping the device into this patch as they belong together
> - Added documentation for maxQueuedRequestsDevice_
> - Fixed an issue with doQueueRequests beeing() called recursively
> 
> Changes in v1:
> - Used a const member variable to carry the maximum number of requests
> - Improved commit message
> - Added docs
> ---
>  include/libcamera/internal/pipeline_handler.h |  4 +-
>  src/libcamera/pipeline_handler.cpp            | 56 +++++++++++++++----
>  2 files changed, 48 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index be017ad47219..e89d6a33e398 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>,
>                         public Object
>  {
>  public:
> -       PipelineHandler(CameraManager *manager);
> +       PipelineHandler(CameraManager *manager,
> +                       unsigned int maxQueuedRequestsDevice = 32);
>         virtual ~PipelineHandler();
>  
>         virtual bool match(DeviceEnumerator *enumerator) = 0;
> @@ -80,6 +81,7 @@ protected:
>         virtual void releaseDevice(Camera *camera);
>  
>         CameraManager *manager_;
> +       const unsigned int maxQueuedRequestsDevice_;
>  
>  private:
>         void unlockMediaDevices();
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index b50eda5e0f86..e8601be108f0 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)
>  /**
>   * \brief Construct a PipelineHandler instance
>   * \param[in] manager The camera manager
> + * \param[in] maxQueuedRequestsDevice The maximum number of requests queued to
> + * the device
>   *
>   * In order to honour the std::enable_shared_from_this<> contract,
>   * PipelineHandler instances shall never be constructed manually, but always
>   * through the PipelineHandlerFactoryBase::create() function.
>   */
> -PipelineHandler::PipelineHandler(CameraManager *manager)
> -       : manager_(manager), useCount_(0)
> +PipelineHandler::PipelineHandler(CameraManager *manager,
> +                                unsigned int maxQueuedRequestsDevice)
> +       : manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),
> +         useCount_(0)
>  {
>  }
>  
> @@ -360,20 +364,28 @@ void PipelineHandler::unlockMediaDevices()
>   */
>  void PipelineHandler::stop(Camera *camera)
>  {
> +       /*
> +        * Take all waiting requests so that they are not requeued in response
> +        * to completeRequest() being called inside stopDevice(). Cancel them
> +        * after the device to keep them in order.
> +        */
> +       Camera::Private *data = camera->_d();
> +       std::queue<Request *> waitingRequests;
> +       waitingRequests.swap(data->waitingRequests_);

Yes, this looks good - solves the issue and maintains the order.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
>         /* Stop the pipeline handler and let the queued requests complete. */
>         stopDevice(camera);
>  
> -       Camera::Private *data = camera->_d();
> -
>         /* Cancel and signal as complete all waiting requests. */
> -       while (!data->waitingRequests_.empty()) {
> -               Request *request = data->waitingRequests_.front();
> -               data->waitingRequests_.pop();
> +       while (!waitingRequests.empty()) {
> +               Request *request = waitingRequests.front();
> +               waitingRequests.pop();
>                 cancelRequest(request);
>         }
>  
>         /* Make sure no requests are pending. */
>         ASSERT(data->queuedRequests_.empty());
> +       ASSERT(data->waitingRequests_.empty());
>  
>         data->requestSequence_ = 0;
>  }
> @@ -430,9 +442,9 @@ void PipelineHandler::registerRequest(Request *request)
>   * requests which have to be prepared to make sure they are ready for being
>   * queued to the pipeline handler.
>   *
> - * The queue of waiting requests is iterated and all prepared requests are
> - * passed to the pipeline handler in the same order they have been queued by
> - * calling this function.
> + * The queue of waiting requests is iterated and up to \a
> + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler
> + * in the same order they have been queued by calling this function.
>   *
>   * If a Request fails during the preparation phase or if the pipeline handler
>   * fails in queuing the request to the hardware the request is cancelled.
> @@ -487,12 +499,19 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>  {
>         Camera::Private *data = camera->_d();
>         while (!data->waitingRequests_.empty()) {
> +               if (data->queuedRequests_.size() == maxQueuedRequestsDevice_)
> +                       break;
> +
>                 Request *request = data->waitingRequests_.front();
>                 if (!request->_d()->prepared_)
>                         break;
>  
> -               doQueueRequest(request);
> +               /*
> +                * Pop the request first, in case doQueueRequests() is called
> +                * recursively from within doQueueRequest()
> +                */
>                 data->waitingRequests_.pop();
> +               doQueueRequest(request);
>         }
>  }
>  
> @@ -568,6 +587,9 @@ void PipelineHandler::completeRequest(Request *request)
>                 data->queuedRequests_.pop_front();
>                 camera->requestComplete(req);
>         }
> +
> +       /* Allow any waiting requests to be queued to the pipeline. */
> +       doQueueRequests(camera);
>  }
>  
>  /**
> @@ -768,6 +790,18 @@ void PipelineHandler::disconnect()
>   * constant for the whole lifetime of the pipeline handler.
>   */
>  
> +/**
> + * \var PipelineHandler::maxQueuedRequestsDevice_
> + * \brief The maximum number of requests the pipeline handler shall queue to the
> + * device
> + *
> + * Some pipeline handlers do not work efficiently with too many requests queued
> + * into the device. Still it is beneficial for the application to circulate more
> + * than the minimum number of buffers. This variable limits the number of
> + * requests that get queued to the device at once. The rest is kept in a
> + * a waiting queue.
> + */
> +
>  /**
>   * \fn PipelineHandler::name()
>   * \brief Retrieve the pipeline handler name
> -- 
> 2.48.1
>

Hi Stefan

On Mon, Jul 07, 2025 at 09:53:36AM +0200, Stefan Klug wrote:
> Add a maxQueuedRequestsDevice constructor parameter to allow pipeline
> handler classes to limit the maximum number of requests that get queued
> to the device in queueRequestDevice().
> 
> The default value is set to an arbitrary number of 32 which is big
> enough for all currently known use cases.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 

Double S-o-B

> ---
> 
> Changes in v2:
> - Moved the fix to properly handle the waitingRequests queue when
>   stopping the device into this patch as they belong together
> - Added documentation for maxQueuedRequestsDevice_
> - Fixed an issue with doQueueRequests beeing() called recursively
> 
> Changes in v1:
> - Used a const member variable to carry the maximum number of requests
> - Improved commit message
> - Added docs
> ---
>  include/libcamera/internal/pipeline_handler.h |  4 +-
>  src/libcamera/pipeline_handler.cpp            | 56 +++++++++++++++----
>  2 files changed, 48 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index be017ad47219..e89d6a33e398 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>,
>  			public Object
>  {
>  public:
> -	PipelineHandler(CameraManager *manager);
> +	PipelineHandler(CameraManager *manager,
> +			unsigned int maxQueuedRequestsDevice = 32);
>  	virtual ~PipelineHandler();
>  
>  	virtual bool match(DeviceEnumerator *enumerator) = 0;
> @@ -80,6 +81,7 @@ protected:
>  	virtual void releaseDevice(Camera *camera);
>  
>  	CameraManager *manager_;
> +	const unsigned int maxQueuedRequestsDevice_;
>  
>  private:
>  	void unlockMediaDevices();
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index b50eda5e0f86..e8601be108f0 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)
>  /**
>   * \brief Construct a PipelineHandler instance
>   * \param[in] manager The camera manager
> + * \param[in] maxQueuedRequestsDevice The maximum number of requests queued to
> + * the device
>   *
>   * In order to honour the std::enable_shared_from_this<> contract,
>   * PipelineHandler instances shall never be constructed manually, but always
>   * through the PipelineHandlerFactoryBase::create() function.
>   */
> -PipelineHandler::PipelineHandler(CameraManager *manager)
> -	: manager_(manager), useCount_(0)
> +PipelineHandler::PipelineHandler(CameraManager *manager,
> +				 unsigned int maxQueuedRequestsDevice)
> +	: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),
> +	  useCount_(0)
>  {
>  }
>  
> @@ -360,20 +364,28 @@ void PipelineHandler::unlockMediaDevices()
>   */
>  void PipelineHandler::stop(Camera *camera)
>  {
> +	/*
> +	 * Take all waiting requests so that they are not requeued in response
> +	 * to completeRequest() being called inside stopDevice(). Cancel them
> +	 * after the device to keep them in order.
> +	 */
> +	Camera::Private *data = camera->_d();
> +	std::queue<Request *> waitingRequests;
> +	waitingRequests.swap(data->waitingRequests_);
> +
>  	/* Stop the pipeline handler and let the queued requests complete. */
>  	stopDevice(camera);
>  
> -	Camera::Private *data = camera->_d();
> -
>  	/* Cancel and signal as complete all waiting requests. */
> -	while (!data->waitingRequests_.empty()) {
> -		Request *request = data->waitingRequests_.front();
> -		data->waitingRequests_.pop();
> +	while (!waitingRequests.empty()) {
> +		Request *request = waitingRequests.front();
> +		waitingRequests.pop();
>  		cancelRequest(request);
>  	}
>  
>  	/* Make sure no requests are pending. */
>  	ASSERT(data->queuedRequests_.empty());
> +	ASSERT(data->waitingRequests_.empty());
>  
>  	data->requestSequence_ = 0;
>  }
> @@ -430,9 +442,9 @@ void PipelineHandler::registerRequest(Request *request)
>   * requests which have to be prepared to make sure they are ready for being
>   * queued to the pipeline handler.
>   *
> - * The queue of waiting requests is iterated and all prepared requests are
> - * passed to the pipeline handler in the same order they have been queued by
> - * calling this function.
> + * The queue of waiting requests is iterated and up to \a
> + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler
> + * in the same order they have been queued by calling this function.
>   *
>   * If a Request fails during the preparation phase or if the pipeline handler
>   * fails in queuing the request to the hardware the request is cancelled.
> @@ -487,12 +499,19 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>  {
>  	Camera::Private *data = camera->_d();
>  	while (!data->waitingRequests_.empty()) {
> +		if (data->queuedRequests_.size() == maxQueuedRequestsDevice_)
> +			break;
> +
>  		Request *request = data->waitingRequests_.front();
>  		if (!request->_d()->prepared_)
>  			break;
>  
> -		doQueueRequest(request);
> +		/*
> +		 * Pop the request first, in case doQueueRequests() is called
> +		 * recursively from within doQueueRequest()
> +		 */

I think what you meant here is, doQueueRequests is called within
completeBuffer().

>  		data->waitingRequests_.pop();
> +		doQueueRequest(request);
>  	}
>  }
>  
> @@ -568,6 +587,9 @@ void PipelineHandler::completeRequest(Request *request)
>  		data->queuedRequests_.pop_front();
>  		camera->requestComplete(req);
>  	}
> +
> +	/* Allow any waiting requests to be queued to the pipeline. */
> +	doQueueRequests(camera);
>  }
>  
>  /**
> @@ -768,6 +790,18 @@ void PipelineHandler::disconnect()
>   * constant for the whole lifetime of the pipeline handler.
>   */
>  
> +/**
> + * \var PipelineHandler::maxQueuedRequestsDevice_
> + * \brief The maximum number of requests the pipeline handler shall queue to the
> + * device
> + *
> + * Some pipeline handlers do not work efficiently with too many requests queued
> + * into the device. Still it is beneficial for the application to circulate more
> + * than the minimum number of buffers. This variable limits the number of
> + * requests that get queued to the device at once. The rest is kept in a
> + * a waiting queue.
> + */
> +

/**
 * \var PipelineHandler::maxQueuedRequestsDevice_
 * \brief The maximum number of requests the pipeline handler shall queue to the
 * device
 *
 * maxQueuedRequestsDevice_ limits the number of request that the
 * pipeline handler shall queue to the underlying hardware, in order to
 * saturate the pipeline with requests. The application may choose to queue
 * as many requests as it desires, however only maxQueuedRequestsDevice_
 * requests will be queued to the hardware at a given point in time. The
 * remaining requests will be kept waiting in the internal waiting
 * queue, to be queued at a later stage.
 */

Reviewed-by: Umang Jain <uajain@igalia.com>


>  /**
>   * \fn PipelineHandler::name()
>   * \brief Retrieve the pipeline handler name
> -- 
> 2.48.1
>

Hi Umang,

Thank you for the review.

Quoting Umang Jain (2025-07-08 05:59:07)
> Hi Stefan
> 
> On Mon, Jul 07, 2025 at 09:53:36AM +0200, Stefan Klug wrote:
> > Add a maxQueuedRequestsDevice constructor parameter to allow pipeline
> > handler classes to limit the maximum number of requests that get queued
> > to the device in queueRequestDevice().
> > 
> > The default value is set to an arbitrary number of 32 which is big
> > enough for all currently known use cases.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> 
> Double S-o-B
> 
> > ---
> > 
> > Changes in v2:
> > - Moved the fix to properly handle the waitingRequests queue when
> >   stopping the device into this patch as they belong together
> > - Added documentation for maxQueuedRequestsDevice_
> > - Fixed an issue with doQueueRequests beeing() called recursively
> > 
> > Changes in v1:
> > - Used a const member variable to carry the maximum number of requests
> > - Improved commit message
> > - Added docs
> > ---
> >  include/libcamera/internal/pipeline_handler.h |  4 +-
> >  src/libcamera/pipeline_handler.cpp            | 56 +++++++++++++++----
> >  2 files changed, 48 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index be017ad47219..e89d6a33e398 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>,
> >                       public Object
> >  {
> >  public:
> > -     PipelineHandler(CameraManager *manager);
> > +     PipelineHandler(CameraManager *manager,
> > +                     unsigned int maxQueuedRequestsDevice = 32);
> >       virtual ~PipelineHandler();
> >  
> >       virtual bool match(DeviceEnumerator *enumerator) = 0;
> > @@ -80,6 +81,7 @@ protected:
> >       virtual void releaseDevice(Camera *camera);
> >  
> >       CameraManager *manager_;
> > +     const unsigned int maxQueuedRequestsDevice_;
> >  
> >  private:
> >       void unlockMediaDevices();
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index b50eda5e0f86..e8601be108f0 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >  /**
> >   * \brief Construct a PipelineHandler instance
> >   * \param[in] manager The camera manager
> > + * \param[in] maxQueuedRequestsDevice The maximum number of requests queued to
> > + * the device
> >   *
> >   * In order to honour the std::enable_shared_from_this<> contract,
> >   * PipelineHandler instances shall never be constructed manually, but always
> >   * through the PipelineHandlerFactoryBase::create() function.
> >   */
> > -PipelineHandler::PipelineHandler(CameraManager *manager)
> > -     : manager_(manager), useCount_(0)
> > +PipelineHandler::PipelineHandler(CameraManager *manager,
> > +                              unsigned int maxQueuedRequestsDevice)
> > +     : manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),
> > +       useCount_(0)
> >  {
> >  }
> >  
> > @@ -360,20 +364,28 @@ void PipelineHandler::unlockMediaDevices()
> >   */
> >  void PipelineHandler::stop(Camera *camera)
> >  {
> > +     /*
> > +      * Take all waiting requests so that they are not requeued in response
> > +      * to completeRequest() being called inside stopDevice(). Cancel them
> > +      * after the device to keep them in order.
> > +      */
> > +     Camera::Private *data = camera->_d();
> > +     std::queue<Request *> waitingRequests;
> > +     waitingRequests.swap(data->waitingRequests_);
> > +
> >       /* Stop the pipeline handler and let the queued requests complete. */
> >       stopDevice(camera);
> >  
> > -     Camera::Private *data = camera->_d();
> > -
> >       /* Cancel and signal as complete all waiting requests. */
> > -     while (!data->waitingRequests_.empty()) {
> > -             Request *request = data->waitingRequests_.front();
> > -             data->waitingRequests_.pop();
> > +     while (!waitingRequests.empty()) {
> > +             Request *request = waitingRequests.front();
> > +             waitingRequests.pop();
> >               cancelRequest(request);
> >       }
> >  
> >       /* Make sure no requests are pending. */
> >       ASSERT(data->queuedRequests_.empty());
> > +     ASSERT(data->waitingRequests_.empty());
> >  
> >       data->requestSequence_ = 0;
> >  }
> > @@ -430,9 +442,9 @@ void PipelineHandler::registerRequest(Request *request)
> >   * requests which have to be prepared to make sure they are ready for being
> >   * queued to the pipeline handler.
> >   *
> > - * The queue of waiting requests is iterated and all prepared requests are
> > - * passed to the pipeline handler in the same order they have been queued by
> > - * calling this function.
> > + * The queue of waiting requests is iterated and up to \a
> > + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler
> > + * in the same order they have been queued by calling this function.
> >   *
> >   * If a Request fails during the preparation phase or if the pipeline handler
> >   * fails in queuing the request to the hardware the request is cancelled.
> > @@ -487,12 +499,19 @@ void PipelineHandler::doQueueRequests(Camera *camera)
> >  {
> >       Camera::Private *data = camera->_d();
> >       while (!data->waitingRequests_.empty()) {
> > +             if (data->queuedRequests_.size() == maxQueuedRequestsDevice_)
> > +                     break;
> > +
> >               Request *request = data->waitingRequests_.front();
> >               if (!request->_d()->prepared_)
> >                       break;
> >  
> > -             doQueueRequest(request);
> > +             /*
> > +              * Pop the request first, in case doQueueRequests() is called
> > +              * recursively from within doQueueRequest()
> > +              */
> 
> I think what you meant here is, doQueueRequests is called within
> completeBuffer().

Actually I meant it the way I wrote it. You are right, that
doQueueRequests() is called within completeBuffer() but it is only
problematic if that completeBuffer() is called inside doQueueRequest()
leading to the recursion.

Best regards,
Stefan

> 
> >               data->waitingRequests_.pop();
> > +             doQueueRequest(request);
> >       }
> >  }
> >  
> > @@ -568,6 +587,9 @@ void PipelineHandler::completeRequest(Request *request)
> >               data->queuedRequests_.pop_front();
> >               camera->requestComplete(req);
> >       }
> > +
> > +     /* Allow any waiting requests to be queued to the pipeline. */
> > +     doQueueRequests(camera);
> >  }
> >  
> >  /**
> > @@ -768,6 +790,18 @@ void PipelineHandler::disconnect()
> >   * constant for the whole lifetime of the pipeline handler.
> >   */
> >  
> > +/**
> > + * \var PipelineHandler::maxQueuedRequestsDevice_
> > + * \brief The maximum number of requests the pipeline handler shall queue to the
> > + * device
> > + *
> > + * Some pipeline handlers do not work efficiently with too many requests queued
> > + * into the device. Still it is beneficial for the application to circulate more
> > + * than the minimum number of buffers. This variable limits the number of
> > + * requests that get queued to the device at once. The rest is kept in a
> > + * a waiting queue.
> > + */
> > +
> 
> /**
>  * \var PipelineHandler::maxQueuedRequestsDevice_
>  * \brief The maximum number of requests the pipeline handler shall queue to the
>  * device
>  *
>  * maxQueuedRequestsDevice_ limits the number of request that the
>  * pipeline handler shall queue to the underlying hardware, in order to
>  * saturate the pipeline with requests. The application may choose to queue
>  * as many requests as it desires, however only maxQueuedRequestsDevice_
>  * requests will be queued to the hardware at a given point in time. The
>  * remaining requests will be kept waiting in the internal waiting
>  * queue, to be queued at a later stage.
>  */
> 
> Reviewed-by: Umang Jain <uajain@igalia.com>
> 
> 
> >  /**
> >   * \fn PipelineHandler::name()
> >   * \brief Retrieve the pipeline handler name
> > -- 
> > 2.48.1
> >