| Message ID | 20250701045805.15201-1-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. Quoting Umang Jain (2025-07-01 06:58:05) > PipelineHandler::queueRequestDevice() returns a negative error code if > the request fails to get queued to the underlying hardware. One such > case would be if the hardware is saturated with requests beyond its > limits, in which case queueRequestDevice() should return -EAGAIN. > > This way, each pipeline handler can keep their limits internal and > decide to start returning -EAGAIN if they foresee their pipelines > queues getting over-saturated with requests. > > Hence, returns error codes from doQueueRequest() so that > doQueueRequests() can make an inform decision whether to pop off the > requests from the internal waiting queue or not. If I got you right, the idea is to replace the MaxQueuedRequestsDevice I added in the other series by this mechanism that return -EAGAIN on saturation. That might actually work equally well... I think I need to try that out. Best regards, Stefan > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > include/libcamera/internal/pipeline_handler.h | 2 +- > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++----- > 2 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 972a2fa6..35252e3a 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -88,7 +88,7 @@ private: > void mediaDeviceDisconnected(MediaDevice *media); > virtual void disconnect(); > > - void doQueueRequest(Request *request); > + int doQueueRequest(Request *request); > void doQueueRequests(); > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index d84dff3c..2ce093ff 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -452,8 +452,10 @@ void PipelineHandler::queueRequest(Request *request) > /** > * \brief Queue one requests to the device > */ > -void PipelineHandler::doQueueRequest(Request *request) > +int PipelineHandler::doQueueRequest(Request *request) > { > + int ret; > + > LIBCAMERA_TRACEPOINT(request_device_queue, request); > > Camera *camera = request->_d()->camera(); > @@ -464,12 +466,14 @@ void PipelineHandler::doQueueRequest(Request *request) > > if (request->_d()->cancelled_) { > completeRequest(request); > - return; > + return -ECANCELED; > } > > - int ret = queueRequestDevice(camera, request); > - if (ret) > + ret = queueRequestDevice(camera, request); > + if (ret && ret != -EAGAIN) > cancelRequest(request); > + > + return ret; > } > > /** > @@ -485,7 +489,9 @@ void PipelineHandler::doQueueRequests() > if (!request->_d()->prepared_) > break; > > - doQueueRequest(request); > + if (doQueueRequest(request) == -EAGAIN) > + break; > + > waitingRequests_.pop(); > } > } > @@ -502,6 +508,10 @@ void PipelineHandler::doQueueRequests() > * parameters will be applied to the frames captured in the buffers provided in > * the request. > * > + * If the underlying hardware pipeline is saturated with requests, this > + * function returns -EAGAIN, so that the \a request stays in the internal > + * waiting queue. > + * > * \context This function is called from the CameraManager thread. > * > * \return 0 on success or a negative error code otherwise > -- > 2.50.0 >
Hi Umang, Quoting Stefan Klug (2025-07-01 09:59:55) > Hi Umang, > > Thank you for the patch. > > Quoting Umang Jain (2025-07-01 06:58:05) > > PipelineHandler::queueRequestDevice() returns a negative error code if > > the request fails to get queued to the underlying hardware. One such > > case would be if the hardware is saturated with requests beyond its > > limits, in which case queueRequestDevice() should return -EAGAIN. > > > > This way, each pipeline handler can keep their limits internal and > > decide to start returning -EAGAIN if they foresee their pipelines > > queues getting over-saturated with requests. > > > > Hence, returns error codes from doQueueRequest() so that > > doQueueRequests() can make an inform decision whether to pop off the > > requests from the internal waiting queue or not. > > If I got you right, the idea is to replace the MaxQueuedRequestsDevice I > added in the other series by this mechanism that return -EAGAIN on > saturation. That might actually work equally well... Another note on that matter. This solution moves the complexity of bookkeeping into the pipeline handler implementation. Could you explain your rationale to prefer this way over the maxQueuedRequests counter? Best regards, Stefan > I think I need to try that out. > > Best regards, > Stefan > > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > --- > > include/libcamera/internal/pipeline_handler.h | 2 +- > > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++----- > > 2 files changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index 972a2fa6..35252e3a 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -88,7 +88,7 @@ private: > > void mediaDeviceDisconnected(MediaDevice *media); > > virtual void disconnect(); > > > > - void doQueueRequest(Request *request); > > + int doQueueRequest(Request *request); > > void doQueueRequests(); > > > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index d84dff3c..2ce093ff 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -452,8 +452,10 @@ void PipelineHandler::queueRequest(Request *request) > > /** > > * \brief Queue one requests to the device > > */ > > -void PipelineHandler::doQueueRequest(Request *request) > > +int PipelineHandler::doQueueRequest(Request *request) > > { > > + int ret; > > + > > LIBCAMERA_TRACEPOINT(request_device_queue, request); > > > > Camera *camera = request->_d()->camera(); > > @@ -464,12 +466,14 @@ void PipelineHandler::doQueueRequest(Request *request) > > > > if (request->_d()->cancelled_) { > > completeRequest(request); > > - return; > > + return -ECANCELED; > > } > > > > - int ret = queueRequestDevice(camera, request); > > - if (ret) > > + ret = queueRequestDevice(camera, request); > > + if (ret && ret != -EAGAIN) > > cancelRequest(request); > > + > > + return ret; > > } > > > > /** > > @@ -485,7 +489,9 @@ void PipelineHandler::doQueueRequests() > > if (!request->_d()->prepared_) > > break; > > > > - doQueueRequest(request); > > + if (doQueueRequest(request) == -EAGAIN) > > + break; > > + > > waitingRequests_.pop(); > > } > > } > > @@ -502,6 +508,10 @@ void PipelineHandler::doQueueRequests() > > * parameters will be applied to the frames captured in the buffers provided in > > * the request. > > * > > + * If the underlying hardware pipeline is saturated with requests, this > > + * function returns -EAGAIN, so that the \a request stays in the internal > > + * waiting queue. > > + * > > * \context This function is called from the CameraManager thread. > > * > > * \return 0 on success or a negative error code otherwise > > -- > > 2.50.0 > >
Hi Stefan, On Tue, Jul 01, 2025 at 10:27:03AM +0200, Stefan Klug wrote: > Hi Umang, > > Quoting Stefan Klug (2025-07-01 09:59:55) > > Hi Umang, > > > > Thank you for the patch. > > > > Quoting Umang Jain (2025-07-01 06:58:05) > > > PipelineHandler::queueRequestDevice() returns a negative error code if > > > the request fails to get queued to the underlying hardware. One such > > > case would be if the hardware is saturated with requests beyond its > > > limits, in which case queueRequestDevice() should return -EAGAIN. > > > > > > This way, each pipeline handler can keep their limits internal and > > > decide to start returning -EAGAIN if they foresee their pipelines > > > queues getting over-saturated with requests. > > > > > > Hence, returns error codes from doQueueRequest() so that > > > doQueueRequests() can make an inform decision whether to pop off the > > > requests from the internal waiting queue or not. > > > > If I got you right, the idea is to replace the MaxQueuedRequestsDevice I > > added in the other series by this mechanism that return -EAGAIN on > > saturation. That might actually work equally well... > Well, I wrote while reviewing the maxQueuedRequestsDevice patch so, I am not sure if this is a replacement (depends on your ultimate goals) or an enhancement in bubbling up return values. > Another note on that matter. This solution moves the complexity of > bookkeeping into the pipeline handler implementation. Could you explain > your rationale to prefer this way over the maxQueuedRequests counter? I don't have any particular rationale for this implementation. For starters, you can even bubble up -EAGAIN (from individual pipelines) to applications to let them know to rate-limit their requests... About maxQueuedRequestsDevice, I think I am missing over-arching design perspective/goals on what needs to be achieved, I have left comments on that threads. And about the complexity, I don't think it's that complex to detect requests queue saturation, so it doesn't matter where it lives, as long as it serves the design. > > Best regards, > Stefan > > > I think I need to try that out. > > > > Best regards, > > Stefan > > > > > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > > --- > > > include/libcamera/internal/pipeline_handler.h | 2 +- > > > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++----- > > > 2 files changed, 16 insertions(+), 6 deletions(-) > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > index 972a2fa6..35252e3a 100644 > > > --- a/include/libcamera/internal/pipeline_handler.h > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > @@ -88,7 +88,7 @@ private: > > > void mediaDeviceDisconnected(MediaDevice *media); > > > virtual void disconnect(); > > > > > > - void doQueueRequest(Request *request); > > > + int doQueueRequest(Request *request); > > > void doQueueRequests(); > > > > > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index d84dff3c..2ce093ff 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -452,8 +452,10 @@ void PipelineHandler::queueRequest(Request *request) > > > /** > > > * \brief Queue one requests to the device > > > */ > > > -void PipelineHandler::doQueueRequest(Request *request) > > > +int PipelineHandler::doQueueRequest(Request *request) > > > { > > > + int ret; > > > + > > > LIBCAMERA_TRACEPOINT(request_device_queue, request); > > > > > > Camera *camera = request->_d()->camera(); > > > @@ -464,12 +466,14 @@ void PipelineHandler::doQueueRequest(Request *request) > > > > > > if (request->_d()->cancelled_) { > > > completeRequest(request); > > > - return; > > > + return -ECANCELED; > > > } > > > > > > - int ret = queueRequestDevice(camera, request); > > > - if (ret) > > > + ret = queueRequestDevice(camera, request); > > > + if (ret && ret != -EAGAIN) > > > cancelRequest(request); > > > + > > > + return ret; > > > } > > > > > > /** > > > @@ -485,7 +489,9 @@ void PipelineHandler::doQueueRequests() > > > if (!request->_d()->prepared_) > > > break; > > > > > > - doQueueRequest(request); > > > + if (doQueueRequest(request) == -EAGAIN) > > > + break; > > > + > > > waitingRequests_.pop(); > > > } > > > } > > > @@ -502,6 +508,10 @@ void PipelineHandler::doQueueRequests() > > > * parameters will be applied to the frames captured in the buffers provided in > > > * the request. > > > * > > > + * If the underlying hardware pipeline is saturated with requests, this > > > + * function returns -EAGAIN, so that the \a request stays in the internal > > > + * waiting queue. > > > + * > > > * \context This function is called from the CameraManager thread. > > > * > > > * \return 0 on success or a negative error code otherwise > > > -- > > > 2.50.0 > > >
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index 972a2fa6..35252e3a 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -88,7 +88,7 @@ private: void mediaDeviceDisconnected(MediaDevice *media); virtual void disconnect(); - void doQueueRequest(Request *request); + int doQueueRequest(Request *request); void doQueueRequests(); std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index d84dff3c..2ce093ff 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -452,8 +452,10 @@ void PipelineHandler::queueRequest(Request *request) /** * \brief Queue one requests to the device */ -void PipelineHandler::doQueueRequest(Request *request) +int PipelineHandler::doQueueRequest(Request *request) { + int ret; + LIBCAMERA_TRACEPOINT(request_device_queue, request); Camera *camera = request->_d()->camera(); @@ -464,12 +466,14 @@ void PipelineHandler::doQueueRequest(Request *request) if (request->_d()->cancelled_) { completeRequest(request); - return; + return -ECANCELED; } - int ret = queueRequestDevice(camera, request); - if (ret) + ret = queueRequestDevice(camera, request); + if (ret && ret != -EAGAIN) cancelRequest(request); + + return ret; } /** @@ -485,7 +489,9 @@ void PipelineHandler::doQueueRequests() if (!request->_d()->prepared_) break; - doQueueRequest(request); + if (doQueueRequest(request) == -EAGAIN) + break; + waitingRequests_.pop(); } } @@ -502,6 +508,10 @@ void PipelineHandler::doQueueRequests() * parameters will be applied to the frames captured in the buffers provided in * the request. * + * If the underlying hardware pipeline is saturated with requests, this + * function returns -EAGAIN, so that the \a request stays in the internal + * waiting queue. + * * \context This function is called from the CameraManager thread. * * \return 0 on success or a negative error code otherwise
PipelineHandler::queueRequestDevice() returns a negative error code if the request fails to get queued to the underlying hardware. One such case would be if the hardware is saturated with requests beyond its limits, in which case queueRequestDevice() should return -EAGAIN. This way, each pipeline handler can keep their limits internal and decide to start returning -EAGAIN if they foresee their pipelines queues getting over-saturated with requests. Hence, returns error codes from doQueueRequest() so that doQueueRequests() can make an inform decision whether to pop off the requests from the internal waiting queue or not. Signed-off-by: Umang Jain <uajain@igalia.com> --- include/libcamera/internal/pipeline_handler.h | 2 +- src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-)