| Message ID | 20250526214224.13631-3-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2025-05-26 22:42:16) > Allow pipeline handler classes to limit the maximum number of requests that > get queued using queueRequestDevice(). > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > include/libcamera/internal/pipeline_handler.h | 5 +++++ > src/libcamera/pipeline_handler.cpp | 3 +++ > 2 files changed, 8 insertions(+) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index dedc29e815fb..86b86d5971dc 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -79,6 +79,11 @@ protected: > virtual bool acquireDevice(Camera *camera); > virtual void releaseDevice(Camera *camera); > > + virtual unsigned int maxQueuedRequestsDevice() const > + { > + return std::numeric_limits<unsigned int>::max(); I wonder if something more small yet big enough would be sane like '16'. But there's not much difference between 'a number big enough' and the 'biggest number' if it should always be overridden. Something like this should come with a corresponding update to the pipeline handler writers guide to clarify what should be considered here. > + } > + > CameraManager *manager_; > > private: > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 14d8602e67d8..1853bca71371 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -490,6 +490,9 @@ void PipelineHandler::doQueueRequests() > > Camera::Private *data = camera->_d(); > while (!data->waitingRequests_.empty()) { Perhaps some verbose explanation here would be helpful to a reader: /* * Limit the number of requests being processed * by the device at one time to keep pipeline * internal depths controlled. The Camera holds * on to requests until the pipeline is ready to * process them. */ > + if (data->queuedRequests_.size() == maxQueuedRequestsDevice()) Would if (data->queuedRequests_.size() >= maxQueuedRequestsDevice()) break; be over cautious given the request can only be added below? > + break; > + > Request *request = data->waitingRequests_.front(); > if (!request->_d()->prepared_) > break; > -- > 2.43.0 >
Hi Stefan, On 5/26/25 23:42, Stefan Klug wrote: > Allow pipeline handler classes to limit the maximum number of requests that > get queued using queueRequestDevice(). > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > include/libcamera/internal/pipeline_handler.h | 5 +++++ > src/libcamera/pipeline_handler.cpp | 3 +++ > 2 files changed, 8 insertions(+) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index dedc29e815fb..86b86d5971dc 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -79,6 +79,11 @@ protected: > virtual bool acquireDevice(Camera *camera); > virtual void releaseDevice(Camera *camera); > > + virtual unsigned int maxQueuedRequestsDevice() const > + { > + return std::numeric_limits<unsigned int>::max(); > + } > + is there a benefit of having a virtual function instead of a variable (initialized to max by default and potentially changed in the subclass constructors)? > CameraManager *manager_; > > private: > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 14d8602e67d8..1853bca71371 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -490,6 +490,9 @@ void PipelineHandler::doQueueRequests() > > 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;
Hi Sven, Stefan On Tue, May 27, 2025 at 05:46:48PM +0200, Sven Püschel wrote: > Hi Stefan, > > On 5/26/25 23:42, Stefan Klug wrote: > > Allow pipeline handler classes to limit the maximum number of requests that > > get queued using queueRequestDevice(). > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > include/libcamera/internal/pipeline_handler.h | 5 +++++ > > src/libcamera/pipeline_handler.cpp | 3 +++ > > 2 files changed, 8 insertions(+) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index dedc29e815fb..86b86d5971dc 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -79,6 +79,11 @@ protected: > > virtual bool acquireDevice(Camera *camera); > > virtual void releaseDevice(Camera *camera); > > + virtual unsigned int maxQueuedRequestsDevice() const > > + { > > + return std::numeric_limits<unsigned int>::max(); > > + } > > + > is there a benefit of having a virtual function instead of a variable > (initialized to max by default and potentially changed in the subclass > constructors)? Just for sake of discussion, should we go in the other direction and make this a pure virtual function in the base class to make sure every pipeline handler provides an implementation ? > > CameraManager *manager_; > > private: > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 14d8602e67d8..1853bca71371 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -490,6 +490,9 @@ void PipelineHandler::doQueueRequests() > > 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;
Hi Jacopo, On 5/30/25 11:02, Jacopo Mondi wrote: > Hi Sven, Stefan > > On Tue, May 27, 2025 at 05:46:48PM +0200, Sven Püschel wrote: >> Hi Stefan, >> >> On 5/26/25 23:42, Stefan Klug wrote: >>> Allow pipeline handler classes to limit the maximum number of requests that >>> get queued using queueRequestDevice(). >>> >>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >>> --- >>> include/libcamera/internal/pipeline_handler.h | 5 +++++ >>> src/libcamera/pipeline_handler.cpp | 3 +++ >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h >>> index dedc29e815fb..86b86d5971dc 100644 >>> --- a/include/libcamera/internal/pipeline_handler.h >>> +++ b/include/libcamera/internal/pipeline_handler.h >>> @@ -79,6 +79,11 @@ protected: >>> virtual bool acquireDevice(Camera *camera); >>> virtual void releaseDevice(Camera *camera); >>> + virtual unsigned int maxQueuedRequestsDevice() const >>> + { >>> + return std::numeric_limits<unsigned int>::max(); >>> + } >>> + >> is there a benefit of having a virtual function instead of a variable >> (initialized to max by default and potentially changed in the subclass >> constructors)? > Just for sake of discussion, should we go in the other direction and > make this a pure virtual function in the base class to make sure every > pipeline handler provides an implementation ? Option 4: Additional constructor parameter which initializes a constant member variable ^-^ My reasons why I prefer a variable: - A function returning a constant just looks weird to me (reminds me of https://xkcd.com/221/ ) - (for a constant) code can assume that it won't change during it's lifetime (thinking about cases like == maxQueueRequestsDevice() pointed out in another comment) - If there is an actual use case requiring a variable number, it's not hard to change it later to a function > >>> CameraManager *manager_; >>> private: >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >>> index 14d8602e67d8..1853bca71371 100644 >>> --- a/src/libcamera/pipeline_handler.cpp >>> +++ b/src/libcamera/pipeline_handler.cpp >>> @@ -490,6 +490,9 @@ void PipelineHandler::doQueueRequests() >>> 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;
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index dedc29e815fb..86b86d5971dc 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -79,6 +79,11 @@ protected: virtual bool acquireDevice(Camera *camera); virtual void releaseDevice(Camera *camera); + virtual unsigned int maxQueuedRequestsDevice() const + { + return std::numeric_limits<unsigned int>::max(); + } + CameraManager *manager_; private: diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 14d8602e67d8..1853bca71371 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -490,6 +490,9 @@ void PipelineHandler::doQueueRequests() 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;
Allow pipeline handler classes to limit the maximum number of requests that get queued using queueRequestDevice(). Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- include/libcamera/internal/pipeline_handler.h | 5 +++++ src/libcamera/pipeline_handler.cpp | 3 +++ 2 files changed, 8 insertions(+)