[RFC,4/4] pipeline: rkisp1: Prperly handle the bufferCount set in the stream configuration

23449 diff mbox series

The bufferCount is reset to a hardcoded value of 4 in RkISP1Path::validate().
Keep the default value of 4 but do not reset it, if it was changed.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 5 +++--
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++----
 src/libcamera/pipeline/rkisp1/rkisp1_path.h   | 4 +---
 3 files changed, 6 insertions(+), 9 deletions(-)

Quoting Stefan Klug (2025-05-26 22:42:18)
> The bufferCount is reset to a hardcoded value of 4 in RkISP1Path::validate().
> Keep the default value of 4 but do not reset it, if it was changed.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 5 +++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++----
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   | 4 +---
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index af9117c83630..ea94bccd252d 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -791,6 +791,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>                         return nullptr;
>  
>                 cfg.colorSpace = colorSpace;
> +               cfg.bufferCount = kPipelineDepth;

Does the user setting this larger now have any effect anywhere at all ?
or does cfg.bufferCount become unused ?

>                 config->addConfiguration(cfg);
>         }
>  
> @@ -1124,14 +1125,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>         }
>  
>         if (data->mainPath_->isEnabled()) {
> -               ret = mainPath_.start();
> +               ret = mainPath_.start(maxQueuedRequestsDevice());
>                 if (ret)
>                         return ret;
>                 actions += [&]() { mainPath_.stop(); };
>         }
>  
>         if (hasSelfPath_ && data->selfPath_->isEnabled()) {
> -               ret = selfPath_.start();
> +               ret = selfPath_.start(maxQueuedRequestsDevice());
>                 if (ret)
>                         return ret;
>         }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 64018dc5b2f4..2f482fcc1834 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -249,7 +249,6 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
>         StreamConfiguration cfg(formats);
>         cfg.pixelFormat = format;
>         cfg.size = streamSize;
> -       cfg.bufferCount = RKISP1_BUFFER_COUNT;
>  
>         return cfg;
>  }
> @@ -383,7 +382,6 @@ RkISP1Path::validate(const CameraSensor *sensor,
>  
>         cfg->size.boundTo(maxResolution);
>         cfg->size.expandTo(minResolution);
> -       cfg->bufferCount = RKISP1_BUFFER_COUNT;
>  
>         V4L2DeviceFormat format;
>         format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);
> @@ -480,7 +478,7 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>         return 0;
>  }
>  
> -int RkISP1Path::start()
> +int RkISP1Path::start(unsigned int bufferCount)
>  {
>         int ret;
>  
> @@ -488,7 +486,7 @@ int RkISP1Path::start()
>                 return -EBUSY;
>  
>         /* \todo Make buffer count user configurable. */
> -       ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
> +       ret = video_->importBuffers(bufferCount);


So - I think we should do some extra digging here.

I think we might actually want to 'import' the V4L2 maximum here (and in
general, always) as that's more about setting up the structures that let
us use externally provided dmabufs and get them into libcamera.

If we now restrict this to '4' but we have '8' separate iterating
buffers we blow through the v4l2-buffer-cache each iteration.

I think the maximum is something like 32 - and this is a small memory
cost to allocote so we should be doing this automatically at the
V4L2VideoDevice::importBuffers() operation.



>         if (ret)
>                 return ret;
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index 430181d371a7..0b60c499ac64 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -58,7 +58,7 @@ public:
>                 return video_->exportBuffers(bufferCount, buffers);
>         }
>  
> -       int start();
> +       int start(unsigned int bufferCount);
>         void stop();
>  
>         int queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); }
> @@ -69,8 +69,6 @@ private:
>         void populateFormats();
>         Size filterSensorResolution(const CameraSensor *sensor);
>  
> -       static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> -
>         const char *name_;
>         bool running_;
>  
> -- 
> 2.43.0
>