[v3,3/5] pipeline: rkisp1: Limit the maximum number of buffers queued in

23841 diff mbox series

To keep the regulation of the algorithms as fast as possible and at the
same time allow more buffers to be allocated, limit the amount of
buffers that get queued into the device to the pipeline depth plus a
tiny margin.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-By: Sven Püschel<s.pueschel@pengutronix.de>

---

Changes in v3:
- Collected tags
- Renamed kPipelineDepth to kRkISP1MaxQueuedRequests and made it static
  constexpr
- Added comment to kRkISP1MaxQueuedRequests
- Improved commit message

Changes in v1:
- Replaced function overload with constructor param
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

On Thu, Jul 17, 2025 at 02:59:23PM +0200, Stefan Klug wrote:
> To keep the regulation of the algorithms as fast as possible and at the
> same time allow more buffers to be allocated, limit the amount of
> buffers that get queued into the device to the pipeline depth plus a
> tiny margin.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Tested-By: Sven Püschel<s.pueschel@pengutronix.de>
> 
> ---
> 
> Changes in v3:
> - Collected tags
> - Renamed kPipelineDepth to kRkISP1MaxQueuedRequests and made it static
>   constexpr
> - Added comment to kRkISP1MaxQueuedRequests
> - Improved commit message
> 
> Changes in v1:
> - Replaced function overload with constructor param
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 675f0a7490a6..7954ea82fd0d 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -155,6 +155,17 @@ private:
>  	Transform combinedTransform_;
>  };
>  
> +namespace {
> +
> +/*
> + * Maximum number of requests that shall be queued into the pipeline to keep
> + * the regulation fast. \todo This needs revisiting as soon as buffers got

nit: \todo on a new line.
> + * decoupled from requests and/or a fast path for controls was implemented.
> + */
> +static constexpr unsigned int kRkISP1MaxQueuedRequests = 4;
> +
> +} // namespace

nit: /* namespace */

Reviewed-by: Umang Jain <uajain@igalia.com>
> +
>  class PipelineHandlerRkISP1 : public PipelineHandler
>  {
>  public:
> @@ -684,7 +695,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>   */
>  
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> -	: PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)
> +	: PipelineHandler(manager, kRkISP1MaxQueuedRequests),
> +	  hasSelfPath_(true), useDewarper_(false)
>  {
>  }
>  
> -- 
> 2.48.1
>

Hi Umang,

Thank you for the review.

Quoting Umang Jain (2025-07-18 07:03:58)
> On Thu, Jul 17, 2025 at 02:59:23PM +0200, Stefan Klug wrote:
> > To keep the regulation of the algorithms as fast as possible and at the
> > same time allow more buffers to be allocated, limit the amount of
> > buffers that get queued into the device to the pipeline depth plus a
> > tiny margin.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Tested-By: Sven Püschel<s.pueschel@pengutronix.de>
> > 
> > ---
> > 
> > Changes in v3:
> > - Collected tags
> > - Renamed kPipelineDepth to kRkISP1MaxQueuedRequests and made it static
> >   constexpr
> > - Added comment to kRkISP1MaxQueuedRequests
> > - Improved commit message
> > 
> > Changes in v1:
> > - Replaced function overload with constructor param
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 675f0a7490a6..7954ea82fd0d 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -155,6 +155,17 @@ private:
> >       Transform combinedTransform_;
> >  };
> >  
> > +namespace {
> > +
> > +/*
> > + * Maximum number of requests that shall be queued into the pipeline to keep
> > + * the regulation fast. \todo This needs revisiting as soon as buffers got
> 
> nit: \todo on a new line.
> > + * decoupled from requests and/or a fast path for controls was implemented.
> > + */
> > +static constexpr unsigned int kRkISP1MaxQueuedRequests = 4;
> > +
> > +} // namespace
> 
> nit: /* namespace */

Good point. That was auto added by checkstyle so I thought that must be
correct. But it doesn't complain on /* */ style comments. 

> 
> Reviewed-by: Umang Jain <uajain@igalia.com>

Thanks,
Stefan

> > +
> >  class PipelineHandlerRkISP1 : public PipelineHandler
> >  {
> >  public:
> > @@ -684,7 +695,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >   */
> >  
> >  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> > -     : PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)
> > +     : PipelineHandler(manager, kRkISP1MaxQueuedRequests),
> > +       hasSelfPath_(true), useDewarper_(false)
> >  {
> >  }
> >  
> > -- 
> > 2.48.1
> >