Message ID | 20250630081126.2384387-3-stefan.klug@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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 > >
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); } /**
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(-)