| 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 > > >
On Tue, Jul 01, 2025 at 03:13:18PM +0530, Umang Jain wrote: > On Tue, Jul 01, 2025 at 10:27:03AM +0200, Stefan Klug wrote: > > Quoting Stefan Klug (2025-07-01 09:59:55) > > > 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... No we can't do that, the call is asynchronous. > 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. If it lives in the pipeline handler the logic will need to be duplicated in all pipeline handlers. Unless there are clear use cases for a logic different than cheking the number of queued requests, I'd rather handle this in the base class. > > > I think I need to try that out. > > > > > > > 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
Hi Umang, Quoting Umang Jain (2025-07-01 11:43:18) > 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. Hm, then I'm not really getting the purpose of this patch. I don't think we should bubble -EAGAIN up to applications as that would be a big break in the behavior of libcamera (and I don't see a reason for user land to force them to rate limit their requests). If we implement -EAGAIN to limit the number of buffers queued to the device, we should not implement maxQueuedRequestsDevice as it would be two implementations for basically the same thing. Could you explain your thoughts? I seem to be missing something. Best regards, Stefan > > > > > 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 > > > >
On Tue, Jul 01, 2025 at 03:20:44PM +0200, Stefan Klug wrote: > Hi Umang, > > Quoting Umang Jain (2025-07-01 11:43:18) > > 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. > > Hm, then I'm not really getting the purpose of this patch. I don't think > we should bubble -EAGAIN up to applications as that would be a big break > in the behavior of libcamera (and I don't see a reason for user land to > force them to rate limit their requests). If we implement -EAGAIN to > limit the number of buffers queued to the device, we should not > implement maxQueuedRequestsDevice as it would be two implementations for > basically the same thing. Could you explain your thoughts? I seem to be > missing something. As Laurent has pointed out in the other reply, -EAGAIN would end up in code duplication *if* the only use case is to keep the request queue in check - from overflow. My idea was to keep the PipelineHandler base simple, and add a facility to let individual pipeline handler dictate - whether or not they want to consume more waiting requests at the moment, by returning -EAGAIN. It could have been based on a no. of factors - the individual pipeline-handler would/can decide. You can go ahead with maxQueuedRequestsDevice as that's the simplest and only use case we have right now. We can always revisit this, if there are clear use cases supporting this case. I don't have any right now. > > Best regards, > Stefan > > > > > > > > > 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(-)