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

23688 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>

---

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            | 20 ++++++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

Hi Stefan,

On Mon, Jun 30, 2025 at 10:11:17AM +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>
> 
> ---
> 
> 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            | 20 ++++++++++++++-----
>  2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
>  {
>  }
>  
> @@ -430,9 +434,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,6 +491,9 @@ 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;
> @@ -568,6 +575,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);

Is this change intended ? I am looking at your original RFC:
https://patchwork.libcamera.org/patch/23447/ which doesn't have this.

I am asking this because, for me, not only this change is orthogonal
to the patch but also, it seems a bit 'off' to try to queue waiting
requests in completeRequest(). Especially considering the fact that
completeRequest() can be called on request-cancellation (probably on
a stop() sequence), and then we try to queue more requests.

Now with this change, we have patch 6/6 which seems to handle the side
effect of this. The stop() sequence is cancelling requests, followed
by completeRequest(), hence to stop doQueueRequests() from queuing
the requests, we need to clear the waiting queue early-on. I feel, this
is a bit inter-twined.

I haven't explored any concrete solutions yet, but I think we need
to first split off this change from this patch atleast.

>  }
>  
>  /**
> -- 
> 2.48.1
>

Hi

2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:
> Hi Stefan,
> 
> On Mon, Jun 30, 2025 at 10:11:17AM +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>
>>
>> ---
>>
>> 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            | 20 ++++++++++++++-----
>>   2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
>>   {
>>   }
>>   
>> @@ -430,9 +434,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,6 +491,9 @@ 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;
>> @@ -568,6 +575,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);
> 
> Is this change intended ? I am looking at your original RFC:
> https://patchwork.libcamera.org/patch/23447/ which doesn't have this.
> 
> I am asking this because, for me, not only this change is orthogonal
> to the patch but also, it seems a bit 'off' to try to queue waiting
> requests in completeRequest(). Especially considering the fact that
> completeRequest() can be called on request-cancellation (probably on
> a stop() sequence), and then we try to queue more requests.

I think the RFC is incomplete, and this addition is required, in order to drain
`waitingRequests_`. Any time an item is removed from `queuedRequests_`, the
`waitingRequests_` queue needs to be checked and the eligible requests need to
be queued to the pipeline handler. Maybe this is not the ideal location since this
also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()
might get a call back into the pipeline handler, which it might not be able to handle.
But I think something like this is needed in any case.


Regards,
Barnabás Pőcze


> 
> Now with this change, we have patch 6/6 which seems to handle the side
> effect of this. The stop() sequence is cancelling requests, followed
> by completeRequest(), hence to stop doQueueRequests() from queuing
> the requests, we need to clear the waiting queue early-on. I feel, this
> is a bit inter-twined.
> 
> I haven't explored any concrete solutions yet, but I think we need
> to first split off this change from this patch atleast.
> 
>>   }
>>   
>>   /**
>> -- 
>> 2.48.1
>>

Hi,

On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:
> Hi
> 
> 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:
> > Hi Stefan,
> > 
> > On Mon, Jun 30, 2025 at 10:11:17AM +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>
> > > 
> > > ---
> > > 
> > > 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            | 20 ++++++++++++++-----
> > >   2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
> > >   {
> > >   }
> > > @@ -430,9 +434,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,6 +491,9 @@ 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;
> > > @@ -568,6 +575,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);
> > 
> > Is this change intended ? I am looking at your original RFC:
> > https://patchwork.libcamera.org/patch/23447/ which doesn't have this.
> > 
> > I am asking this because, for me, not only this change is orthogonal
> > to the patch but also, it seems a bit 'off' to try to queue waiting
> > requests in completeRequest(). Especially considering the fact that
> > completeRequest() can be called on request-cancellation (probably on
> > a stop() sequence), and then we try to queue more requests.
> 
> I think the RFC is incomplete, and this addition is required, in order to drain
> `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the

Why the waitingRequests_ needs to be drained? It simply needs to be
cancelled/cleared on stop(), as a cleanup and to prevent it
(accidently) getting queued down to the hardware..

> `waitingRequests_` queue needs to be checked and the eligible requests need to
> be queued to the pipeline handler. Maybe this is not the ideal location since this
> also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()
> might get a call back into the pipeline handler, which it might not be able to handle.
> But I think something like this is needed in any case.

I am not sure what use-case/sequence led to this particular change
(description somewhere would have been nice), but to me,
doQueueRequests() does the right thing i.e. being executed when the
request emits 'prepared', which is chained to
PipelineHandler::queueRequest().

In normal/most situation,

completeRequest() -> queuedRequests_.pop() -> Request::reuse()
-> queueRequest() -> prepared signal emit -> doQueueRequests()
-> waitingRequest_.front() queued.

So, yes changes to queuedRequest_ does end up checking waitingRequests_
in the end, as mentioned above.

> 
> 
> Regards,
> Barnabás Pőcze
> 
> 
> > 
> > Now with this change, we have patch 6/6 which seems to handle the side
> > effect of this. The stop() sequence is cancelling requests, followed
> > by completeRequest(), hence to stop doQueueRequests() from queuing
> > the requests, we need to clear the waiting queue early-on. I feel, this
> > is a bit inter-twined.
> > 
> > I haven't explored any concrete solutions yet, but I think we need
> > to first split off this change from this patch atleast.
> > 
> > >   }
> > >   /**
> > > -- 
> > > 2.48.1
> > > 
>

Hi

2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:
> Hi,
> 
> On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:
>> Hi
>>
>> 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:
>>> Hi Stefan,
>>>
>>> On Mon, Jun 30, 2025 at 10:11:17AM +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>
>>>>
>>>> ---
>>>>
>>>> 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            | 20 ++++++++++++++-----
>>>>    2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
>>>>    {
>>>>    }
>>>> @@ -430,9 +434,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,6 +491,9 @@ 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;
>>>> @@ -568,6 +575,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);
>>>
>>> Is this change intended ? I am looking at your original RFC:
>>> https://patchwork.libcamera.org/patch/23447/ which doesn't have this.
>>>
>>> I am asking this because, for me, not only this change is orthogonal
>>> to the patch but also, it seems a bit 'off' to try to queue waiting
>>> requests in completeRequest(). Especially considering the fact that
>>> completeRequest() can be called on request-cancellation (probably on
>>> a stop() sequence), and then we try to queue more requests.
>>
>> I think the RFC is incomplete, and this addition is required, in order to drain
>> `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the
> 
> Why the waitingRequests_ needs to be drained? It simply needs to be
> cancelled/cleared on stop(), as a cleanup and to prevent it
> (accidently) getting queued down to the hardware..

Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,
and waits for all of them to complete, then stops the camera. If the requests
are all queued before the first one could be completed, then `waitingRequests_`
will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`
requests. If the `waitingRequests_` is not processed on completion, then it
will stay there forever, and it will never complete. (Unless I am missing something?)


Regards,
Barnabás Pőcze


> 
>> `waitingRequests_` queue needs to be checked and the eligible requests need to
>> be queued to the pipeline handler. Maybe this is not the ideal location since this
>> also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()
>> might get a call back into the pipeline handler, which it might not be able to handle.
>> But I think something like this is needed in any case.
> 
> I am not sure what use-case/sequence led to this particular change
> (description somewhere would have been nice), but to me,
> doQueueRequests() does the right thing i.e. being executed when the
> request emits 'prepared', which is chained to
> PipelineHandler::queueRequest().
> 
> In normal/most situation,
> 
> completeRequest() -> queuedRequests_.pop() -> Request::reuse()
> -> queueRequest() -> prepared signal emit -> doQueueRequests()
> -> waitingRequest_.front() queued.
> 
> So, yes changes to queuedRequest_ does end up checking waitingRequests_
> in the end, as mentioned above.
> 
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
>>>
>>> Now with this change, we have patch 6/6 which seems to handle the side
>>> effect of this. The stop() sequence is cancelling requests, followed
>>> by completeRequest(), hence to stop doQueueRequests() from queuing
>>> the requests, we need to clear the waiting queue early-on. I feel, this
>>> is a bit inter-twined.
>>>
>>> I haven't explored any concrete solutions yet, but I think we need
>>> to first split off this change from this patch atleast.
>>>
>>>>    }
>>>>    /**
>>>> -- 
>>>> 2.48.1
>>>>
>>

On Mon, Jun 30, 2025 at 02:43:21PM +0200, Barnabás Pőcze wrote:
> Hi
> 
> 2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:
> > Hi,
> > 
> > On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:
> > > Hi
> > > 
> > > 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:
> > > > Hi Stefan,
> > > > 
> > > > On Mon, Jun 30, 2025 at 10:11:17AM +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>
> > > > > 
> > > > > ---
> > > > > 
> > > > > 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            | 20 ++++++++++++++-----
> > > > >    2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
> > > > >    {
> > > > >    }
> > > > > @@ -430,9 +434,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,6 +491,9 @@ 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;
> > > > > @@ -568,6 +575,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);
> > > > 
> > > > Is this change intended ? I am looking at your original RFC:
> > > > https://patchwork.libcamera.org/patch/23447/ which doesn't have this.
> > > > 
> > > > I am asking this because, for me, not only this change is orthogonal
> > > > to the patch but also, it seems a bit 'off' to try to queue waiting
> > > > requests in completeRequest(). Especially considering the fact that
> > > > completeRequest() can be called on request-cancellation (probably on
> > > > a stop() sequence), and then we try to queue more requests.
> > > 
> > > I think the RFC is incomplete, and this addition is required, in order to drain
> > > `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the
> > 
> > Why the waitingRequests_ needs to be drained? It simply needs to be
> > cancelled/cleared on stop(), as a cleanup and to prevent it
> > (accidently) getting queued down to the hardware..
> 
> Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,
> and waits for all of them to complete, then stops the camera. If the requests
> are all queued before the first one could be completed, then `waitingRequests_`
> will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`
> requests. If the `waitingRequests_` is not processed on completion, then it
> will stay there forever, and it will never complete. (Unless I am missing something?)

In your scenario, the last request 'maxQueuedRequestsDevice_ + 1`th
in waitingRequests_ will complete on calling stop(). It will be complete
by PipelineHandler::cancelRequest().

PipelineHandler::queueRequest() documentation also states:

 * 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. 

Does waitingRequests_ dwould fall in: "fails in queuing the request to
the hardware" category? Becuase the user queued requests too fast and
also beyond maxQueuedRequestsDevice_ ?

> 
> 
> Regards,
> Barnabás Pőcze
> 
> 
> > 
> > > `waitingRequests_` queue needs to be checked and the eligible requests need to
> > > be queued to the pipeline handler. Maybe this is not the ideal location since this
> > > also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()
> > > might get a call back into the pipeline handler, which it might not be able to handle.
> > > But I think something like this is needed in any case.
> > 
> > I am not sure what use-case/sequence led to this particular change
> > (description somewhere would have been nice), but to me,
> > doQueueRequests() does the right thing i.e. being executed when the
> > request emits 'prepared', which is chained to
> > PipelineHandler::queueRequest().
> > 
> > In normal/most situation,
> > 
> > completeRequest() -> queuedRequests_.pop() -> Request::reuse()
> > -> queueRequest() -> prepared signal emit -> doQueueRequests()
> > -> waitingRequest_.front() queued.
> > 
> > So, yes changes to queuedRequest_ does end up checking waitingRequests_
> > in the end, as mentioned above.
> > 
> > > 
> > > 
> > > Regards,
> > > Barnabás Pőcze
> > > 
> > > 
> > > > 
> > > > Now with this change, we have patch 6/6 which seems to handle the side
> > > > effect of this. The stop() sequence is cancelling requests, followed
> > > > by completeRequest(), hence to stop doQueueRequests() from queuing
> > > > the requests, we need to clear the waiting queue early-on. I feel, this
> > > > is a bit inter-twined.
> > > > 
> > > > I haven't explored any concrete solutions yet, but I think we need
> > > > to first split off this change from this patch atleast.
> > > > 
> > > > >    }
> > > > >    /**
> > > > > -- 
> > > > > 2.48.1
> > > > > 
> > > 
>

2025. 06. 30. 17:03 keltezéssel, Umang Jain írta:
> On Mon, Jun 30, 2025 at 02:43:21PM +0200, Barnabás Pőcze wrote:
>> Hi
>>
>> 2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:
>>> Hi,
>>>
>>> On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:
>>>> Hi
>>>>
>>>> 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:
>>>>> Hi Stefan,
>>>>>
>>>>> On Mon, Jun 30, 2025 at 10:11:17AM +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>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> 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            | 20 ++++++++++++++-----
>>>>>>     2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
>>>>>>     {
>>>>>>     }
>>>>>> @@ -430,9 +434,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,6 +491,9 @@ 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;
>>>>>> @@ -568,6 +575,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);
>>>>>
>>>>> Is this change intended ? I am looking at your original RFC:
>>>>> https://patchwork.libcamera.org/patch/23447/ which doesn't have this.
>>>>>
>>>>> I am asking this because, for me, not only this change is orthogonal
>>>>> to the patch but also, it seems a bit 'off' to try to queue waiting
>>>>> requests in completeRequest(). Especially considering the fact that
>>>>> completeRequest() can be called on request-cancellation (probably on
>>>>> a stop() sequence), and then we try to queue more requests.
>>>>
>>>> I think the RFC is incomplete, and this addition is required, in order to drain
>>>> `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the
>>>
>>> Why the waitingRequests_ needs to be drained? It simply needs to be
>>> cancelled/cleared on stop(), as a cleanup and to prevent it
>>> (accidently) getting queued down to the hardware..
>>
>> Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,
>> and waits for all of them to complete, then stops the camera. If the requests
>> are all queued before the first one could be completed, then `waitingRequests_`
>> will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`
>> requests. If the `waitingRequests_` is not processed on completion, then it
>> will stay there forever, and it will never complete. (Unless I am missing something?)
> 
> In your scenario, the last request 'maxQueuedRequestsDevice_ + 1`th
> in waitingRequests_ will complete on calling stop(). It will be complete
> by PipelineHandler::cancelRequest().

In my scenario `Camera::stop()` would only be called after the completion of the
`maxQueuedRequestsDevice_ + 1` requests, which never happens because the last
request stays in `waitingRequests_` without ever being queued.


> 
> PipelineHandler::queueRequest() documentation also states:
> 
>   * 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.
> 
> Does waitingRequests_ dwould fall in: "fails in queuing the request to
> the hardware" category? Becuase the user queued requests too fast and
> also beyond maxQueuedRequestsDevice_ ?

My assumption was that the point of this patch is to allow an application to
queue as many requests as it wants, and let libcamera deal with the limits, etc.


Regards,
Barnabás Pőcze

> 
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
>>>
>>>> `waitingRequests_` queue needs to be checked and the eligible requests need to
>>>> be queued to the pipeline handler. Maybe this is not the ideal location since this
>>>> also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()
>>>> might get a call back into the pipeline handler, which it might not be able to handle.
>>>> But I think something like this is needed in any case.
>>>
>>> I am not sure what use-case/sequence led to this particular change
>>> (description somewhere would have been nice), but to me,
>>> doQueueRequests() does the right thing i.e. being executed when the
>>> request emits 'prepared', which is chained to
>>> PipelineHandler::queueRequest().
>>>
>>> In normal/most situation,
>>>
>>> completeRequest() -> queuedRequests_.pop() -> Request::reuse()
>>> -> queueRequest() -> prepared signal emit -> doQueueRequests()
>>> -> waitingRequest_.front() queued.
>>>
>>> So, yes changes to queuedRequest_ does end up checking waitingRequests_
>>> in the end, as mentioned above.
>>>
>>>>
>>>>
>>>> Regards,
>>>> Barnabás Pőcze
>>>>
>>>>
>>>>>
>>>>> Now with this change, we have patch 6/6 which seems to handle the side
>>>>> effect of this. The stop() sequence is cancelling requests, followed
>>>>> by completeRequest(), hence to stop doQueueRequests() from queuing
>>>>> the requests, we need to clear the waiting queue early-on. I feel, this
>>>>> is a bit inter-twined.
>>>>>
>>>>> I haven't explored any concrete solutions yet, but I think we need
>>>>> to first split off this change from this patch atleast.
>>>>>
>>>>>>     }
>>>>>>     /**
>>>>>> -- 
>>>>>> 2.48.1
>>>>>>
>>>>
>>

On Mon, Jun 30, 2025 at 05:21:59PM +0200, Barnabás Pőcze wrote:
> 2025. 06. 30. 17:03 keltezéssel, Umang Jain írta:
> > On Mon, Jun 30, 2025 at 02:43:21PM +0200, Barnabás Pőcze wrote:
> > > Hi
> > > 
> > > 2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:
> > > > Hi,
> > > > 
> > > > On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:
> > > > > Hi
> > > > > 
> > > > > 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:
> > > > > > Hi Stefan,
> > > > > > 
> > > > > > On Mon, Jun 30, 2025 at 10:11:17AM +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>
> > > > > > > 
> > > > > > > ---
> > > > > > > 
> > > > > > > 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            | 20 ++++++++++++++-----
> > > > > > >     2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
> > > > > > >     {
> > > > > > >     }
> > > > > > > @@ -430,9 +434,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,6 +491,9 @@ 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;
> > > > > > > @@ -568,6 +575,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);
> > > > > > 
> > > > > > Is this change intended ? I am looking at your original RFC:
> > > > > > https://patchwork.libcamera.org/patch/23447/ which doesn't have this.
> > > > > > 
> > > > > > I am asking this because, for me, not only this change is orthogonal
> > > > > > to the patch but also, it seems a bit 'off' to try to queue waiting
> > > > > > requests in completeRequest(). Especially considering the fact that
> > > > > > completeRequest() can be called on request-cancellation (probably on
> > > > > > a stop() sequence), and then we try to queue more requests.
> > > > > 
> > > > > I think the RFC is incomplete, and this addition is required, in order to drain
> > > > > `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the
> > > > 
> > > > Why the waitingRequests_ needs to be drained? It simply needs to be
> > > > cancelled/cleared on stop(), as a cleanup and to prevent it
> > > > (accidently) getting queued down to the hardware..
> > > 
> > > Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,
> > > and waits for all of them to complete, then stops the camera. If the requests
> > > are all queued before the first one could be completed, then `waitingRequests_`
> > > will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`
> > > requests. If the `waitingRequests_` is not processed on completion, then it
> > > will stay there forever, and it will never complete. (Unless I am missing something?)
> > 
> > In your scenario, the last request 'maxQueuedRequestsDevice_ + 1`th
> > in waitingRequests_ will complete on calling stop(). It will be complete
> > by PipelineHandler::cancelRequest().
> 
> In my scenario `Camera::stop()` would only be called after the completion of the
> `maxQueuedRequestsDevice_ + 1` requests, which never happens because the last
> request stays in `waitingRequests_` without ever being queued.
> 

Is this a design goal we are after - Queuing exactly (x) requests to get
(x) requests out? I don't know how much it is feasible as of today,
given the multi-stage pipelines we have today.

But if this is a design target, it should clearly stated.
> 
> > 
> > PipelineHandler::queueRequest() documentation also states:
> > 
> >   * 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.
> > 
> > Does waitingRequests_ dwould fall in: "fails in queuing the request to
> > the hardware" category? Becuase the user queued requests too fast and
> > also beyond maxQueuedRequestsDevice_ ?
> 
> My assumption was that the point of this patch is to allow an application to
> queue as many requests as it wants, and let libcamera deal with the limits, etc.

Ditto.
> 
> 
> Regards,
> Barnabás Pőcze
> 
> > 
> > > 
> > > 
> > > Regards,
> > > Barnabás Pőcze
> > > 
> > > 
> > > > 
> > > > > `waitingRequests_` queue needs to be checked and the eligible requests need to
> > > > > be queued to the pipeline handler. Maybe this is not the ideal location since this
> > > > > also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()
> > > > > might get a call back into the pipeline handler, which it might not be able to handle.
> > > > > But I think something like this is needed in any case.
> > > > 
> > > > I am not sure what use-case/sequence led to this particular change
> > > > (description somewhere would have been nice), but to me,
> > > > doQueueRequests() does the right thing i.e. being executed when the
> > > > request emits 'prepared', which is chained to
> > > > PipelineHandler::queueRequest().
> > > > 
> > > > In normal/most situation,
> > > > 
> > > > completeRequest() -> queuedRequests_.pop() -> Request::reuse()
> > > > -> queueRequest() -> prepared signal emit -> doQueueRequests()
> > > > -> waitingRequest_.front() queued.
> > > > 
> > > > So, yes changes to queuedRequest_ does end up checking waitingRequests_
> > > > in the end, as mentioned above.
> > > > 
> > > > > 
> > > > > 
> > > > > Regards,
> > > > > Barnabás Pőcze
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Now with this change, we have patch 6/6 which seems to handle the side
> > > > > > effect of this. The stop() sequence is cancelling requests, followed
> > > > > > by completeRequest(), hence to stop doQueueRequests() from queuing
> > > > > > the requests, we need to clear the waiting queue early-on. I feel, this
> > > > > > is a bit inter-twined.
> > > > > > 
> > > > > > I haven't explored any concrete solutions yet, but I think we need
> > > > > > to first split off this change from this patch atleast.
> > > > > > 
> > > > > > >     }
> > > > > > >     /**
> > > > > > > -- 
> > > > > > > 2.48.1
> > > > > > > 
> > > > > 
> > > 
>

Hi Umang,

Quoting Umang Jain (2025-07-01 07:27:16)
> On Mon, Jun 30, 2025 at 05:21:59PM +0200, Barnabás Pőcze wrote:
> > 2025. 06. 30. 17:03 keltezéssel, Umang Jain írta:
> > > On Mon, Jun 30, 2025 at 02:43:21PM +0200, Barnabás Pőcze wrote:
> > > > Hi
> > > > 
> > > > 2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:
> > > > > Hi,
> > > > > 
> > > > > On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:
> > > > > > Hi
> > > > > > 
> > > > > > 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:
> > > > > > > Hi Stefan,
> > > > > > > 
> > > > > > > On Mon, Jun 30, 2025 at 10:11:17AM +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>
> > > > > > > > 
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > 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            | 20 ++++++++++++++-----
> > > > > > > >     2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
> > > > > > > >     {
> > > > > > > >     }
> > > > > > > > @@ -430,9 +434,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,6 +491,9 @@ 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;
> > > > > > > > @@ -568,6 +575,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);
> > > > > > > 
> > > > > > > Is this change intended ? I am looking at your original RFC:
> > > > > > > https://patchwork.libcamera.org/patch/23447/ which doesn't have this.
> > > > > > > 
> > > > > > > I am asking this because, for me, not only this change is orthogonal
> > > > > > > to the patch but also, it seems a bit 'off' to try to queue waiting
> > > > > > > requests in completeRequest(). Especially considering the fact that
> > > > > > > completeRequest() can be called on request-cancellation (probably on
> > > > > > > a stop() sequence), and then we try to queue more requests.
> > > > > > 
> > > > > > I think the RFC is incomplete, and this addition is required, in order to drain
> > > > > > `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the

Yes, exactly. And it opened a bug that got addressed in patch 6/6. Think
of an application queuing 100 requests in advance (That really starts to
make sense when we split buffers from controls). In that case there
would be 96 requests in waitingRequests_ and these need to be fed into
the pipeline handler as soon as there is room (which is after
completeRequest()) 

> > > > > 
> > > > > Why the waitingRequests_ needs to be drained? It simply needs to be
> > > > > cancelled/cleared on stop(), as a cleanup and to prevent it
> > > > > (accidently) getting queued down to the hardware..
> > > > 
> > > > Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,
> > > > and waits for all of them to complete, then stops the camera. If the requests
> > > > are all queued before the first one could be completed, then `waitingRequests_`
> > > > will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`
> > > > requests. If the `waitingRequests_` is not processed on completion, then it
> > > > will stay there forever, and it will never complete. (Unless I am missing something?)
> > > 
> > > In your scenario, the last request 'maxQueuedRequestsDevice_ + 1`th
> > > in waitingRequests_ will complete on calling stop(). It will be complete
> > > by PipelineHandler::cancelRequest().
> > 
> > In my scenario `Camera::stop()` would only be called after the completion of the
> > `maxQueuedRequestsDevice_ + 1` requests, which never happens because the last
> > request stays in `waitingRequests_` without ever being queued.
> > 
> 
> Is this a design goal we are after - Queuing exactly (x) requests to get
> (x) requests out? I don't know how much it is feasible as of today,
> given the multi-stage pipelines we have today.

I think I'm missing something here. The idea of this series is to allow
to queue more than pipeline depth requests (practically an arbitrary
number) into libcamera. The pipeline handler decides how many requests
it wants to consume. All the others stay in the waitingQueue until there
is room again in the pipeline handler. Maybe I'm missing a bigger
concept that I wasn't aware of.

Best regards,
Stefan

> 
> But if this is a design target, it should clearly stated.
> > 
> > > 
> > > PipelineHandler::queueRequest() documentation also states:
> > > 
> > >   * 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.
> > > 
> > > Does waitingRequests_ dwould fall in: "fails in queuing the request to
> > > the hardware" category? Becuase the user queued requests too fast and
> > > also beyond maxQueuedRequestsDevice_ ?
> > 
> > My assumption was that the point of this patch is to allow an application to
> > queue as many requests as it wants, and let libcamera deal with the limits, etc.
> 
> Ditto.
> > 
> > 
> > Regards,
> > Barnabás Pőcze
> > 
> > > 
> > > > 
> > > > 
> > > > Regards,
> > > > Barnabás Pőcze
> > > > 
> > > > 
> > > > > 
> > > > > > `waitingRequests_` queue needs to be checked and the eligible requests need to
> > > > > > be queued to the pipeline handler. Maybe this is not the ideal location since this
> > > > > > also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()
> > > > > > might get a call back into the pipeline handler, which it might not be able to handle.
> > > > > > But I think something like this is needed in any case.
> > > > > 
> > > > > I am not sure what use-case/sequence led to this particular change
> > > > > (description somewhere would have been nice), but to me,
> > > > > doQueueRequests() does the right thing i.e. being executed when the
> > > > > request emits 'prepared', which is chained to
> > > > > PipelineHandler::queueRequest().
> > > > > 
> > > > > In normal/most situation,
> > > > > 
> > > > > completeRequest() -> queuedRequests_.pop() -> Request::reuse()
> > > > > -> queueRequest() -> prepared signal emit -> doQueueRequests()
> > > > > -> waitingRequest_.front() queued.
> > > > > 
> > > > > So, yes changes to queuedRequest_ does end up checking waitingRequests_
> > > > > in the end, as mentioned above.
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Regards,
> > > > > > Barnabás Pőcze
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Now with this change, we have patch 6/6 which seems to handle the side
> > > > > > > effect of this. The stop() sequence is cancelling requests, followed
> > > > > > > by completeRequest(), hence to stop doQueueRequests() from queuing
> > > > > > > the requests, we need to clear the waiting queue early-on. I feel, this
> > > > > > > is a bit inter-twined.
> > > > > > > 
> > > > > > > I haven't explored any concrete solutions yet, but I think we need
> > > > > > > to first split off this change from this patch atleast.
> > > > > > > 
> > > > > > > >     }
> > > > > > > >     /**
> > > > > > > > -- 
> > > > > > > > 2.48.1
> > > > > > > > 
> > > > > > 
> > > > 
> >

Hi,

Small follow ups questions:

On Mon, Jun 30, 2025 at 10:11:17AM +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.

Is the default:32, some how derived from VB2_MAX_FRAME ? 
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> 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            | 20 ++++++++++++++-----
>  2 files changed, 18 insertions(+), 6 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);

Is clamping needed, or Pipeline handlers are free to set
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 dc4086aa9bb5..383c6ad0c4aa 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)
>  {
>  }
>  
> @@ -430,9 +434,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,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>  {
>  	Camera::Private *data = camera->_d();
>  	while (!data->waitingRequests_.empty()) {
> +		if (data->queuedRequests_.size() == maxQueuedRequestsDevice_)
> +			break;
> +

Thought exercise probably, does this make sense?

 +		if (data->queuedRequests_.size() == data->maxQueuedRequestsDevice_)
 +			break;

i.e. maxQueuedRequestsDevice_ is per-camera pipeline, and not per
pipeline-handler. I do not have concrete use cases, but if forced to
think:

camera0: sensor <> (de)/(se)rializer <> mipi-csi0 <> isp0 and
camera1: sensor <> mipi-csi1 <> isp1

on i.MX8MPi/pi5, and serializers components have other (lesser) queue
limits?

>  		Request *request = data->waitingRequests_.front();
>  		if (!request->_d()->prepared_)
>  			break;
> @@ -568,6 +575,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);
>  }
>  
>  /**
> -- 
> 2.48.1
>

Hi Stefan,

On 6/30/25 10:11, 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>
>
> ---
>
> 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            | 20 ++++++++++++++-----
>   2 files changed, 18 insertions(+), 6 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 dc4086aa9bb5..383c6ad0c4aa 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)
>   {
>   }
>   
> @@ -430,9 +434,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,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>   {
>   	Camera::Private *data = camera->_d();
>   	while (!data->waitingRequests_.empty()) {
> +		if (data->queuedRequests_.size() == maxQueuedRequestsDevice_)

I'd suggest to replace == with >=

I think this was already mentioned in the RFC patchset. Or was there a 
reason against this? Then I'd suggest adding an 
ASSERT(data->queuedRequests_.size() <= maxQueuedRequestsDevice_)

Sincerely Sven

> +			break;
> +
>   		Request *request = data->waitingRequests_.front();
>   		if (!request->_d()->prepared_)
>   			break;
> @@ -568,6 +575,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);
>   }
>   
>   /**

Hi Umang,

Thanks for the feedback,

Quoting Umang Jain (2025-07-02 08:40:10)
> Hi,
> 
> Small follow ups questions:
> 
> On Mon, Jun 30, 2025 at 10:11:17AM +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.
> 
> Is the default:32, some how derived from VB2_MAX_FRAME ? 

Merely by coincidence.

> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > 
> > 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            | 20 ++++++++++++++-----
> >  2 files changed, 18 insertions(+), 6 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);
> 
> Is clamping needed, or Pipeline handlers are free to set
> maxQueuedRequestsDevice > 32 ?

I think a pipeline handler should have the knowledge if it is capable of
handling > 32 requests. I wouldn't limit that.

> 
> >       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 dc4086aa9bb5..383c6ad0c4aa 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)
> >  {
> >  }
> >  
> > @@ -430,9 +434,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,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)
> >  {
> >       Camera::Private *data = camera->_d();
> >       while (!data->waitingRequests_.empty()) {
> > +             if (data->queuedRequests_.size() == maxQueuedRequestsDevice_)
> > +                     break;
> > +
> 
> Thought exercise probably, does this make sense?
> 
>  +              if (data->queuedRequests_.size() == data->maxQueuedRequestsDevice_)
>  +                      break;
> 
> i.e. maxQueuedRequestsDevice_ is per-camera pipeline, and not per
> pipeline-handler. I do not have concrete use cases, but if forced to
> think:
> 
> camera0: sensor <> (de)/(se)rializer <> mipi-csi0 <> isp0 and
> camera1: sensor <> mipi-csi1 <> isp1

This is a valid objection and when we strive for the smallest possible
request count it might be camera specific. Currently
maxQueuedRequestsDevice has a bit of a security margin above the
theoretical minimum and I think that is fine. I expect the pipeline
handlers to further improve in the context of PFC. Then this limit might
not be needed anymore at all. So I'd stick with the simpler solution for
now and see where need arises on the longer run.

Best regards,
Stefan

> 
> on i.MX8MPi/pi5, and serializers components have other (lesser) queue
> limits?
> 
> >               Request *request = data->waitingRequests_.front();
> >               if (!request->_d()->prepared_)
> >                       break;
> > @@ -568,6 +575,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);
> >  }
> >  
> >  /**
> > -- 
> > 2.48.1
> >