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