libcamera: pipeline_hander: Return -EAGAIN if pipeline is saturated
diff mbox series

Message ID 20250701045805.15201-1-uajain@igalia.com
State New
Headers show
Series
  • libcamera: pipeline_hander: Return -EAGAIN if pipeline is saturated
Related show

Commit Message

Umang Jain July 1, 2025, 4:58 a.m. UTC
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(-)

Comments

Stefan Klug July 1, 2025, 7:59 a.m. UTC | #1
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
>
Stefan Klug July 1, 2025, 8:27 a.m. UTC | #2
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
> >
Umang Jain July 1, 2025, 9:43 a.m. UTC | #3
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
> > >
Laurent Pinchart July 1, 2025, 11:22 a.m. UTC | #4
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
Stefan Klug July 1, 2025, 1:20 p.m. UTC | #5
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
> > > >
Umang Jain July 1, 2025, 3:44 p.m. UTC | #6
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
> > > > >

Patch
diff mbox series

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