[v2,4/5] pipeline: rkisp1: Properly handle the bufferCount set in the stream configuration

23747 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.  This allows the user to set bufferCount to an
arbitrary number of buffers which then can be allocated for example by
the FrameBufferAllocator. Internally maxQueuedRequestsDevice_ is used as
buffer count which is the maximum number of buffers queued to the device
at once.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v1:
- Removed todo comment that was solved by this change
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 5 +++--
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 ++-----
 src/libcamera/pipeline/rkisp1/rkisp1_path.h   | 4 +---
 3 files changed, 6 insertions(+), 10 deletions(-)

Quoting Stefan Klug (2025-07-07 08:53:38)
> 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.  This allows the user to set bufferCount to an
> arbitrary number of buffers which then can be allocated for example by
> the FrameBufferAllocator. Internally maxQueuedRequestsDevice_ is used as
> buffer count which is the maximum number of buffers queued to the device
> at once.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v1:
> - Removed todo comment that was solved by this change
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 5 +++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 ++-----
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   | 4 +---
>  3 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index fff42359cbff..bdaeb935ee06 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -790,6 +790,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>                         return nullptr;
>  
>                 cfg.colorSpace = colorSpace;
> +               cfg.bufferCount = kPipelineDepth;
>                 config->addConfiguration(cfg);
>         }
>  
> @@ -1123,14 +1124,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..8ea5500d4080 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,15 +478,14 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>         return 0;
>  }
>  
> -int RkISP1Path::start()
> +int RkISP1Path::start(unsigned int bufferCount)
>  {
>         int ret;
>  
>         if (running_)
>                 return -EBUSY;
>  
> -       /* \todo Make buffer count user configurable. */
> -       ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
> +       ret = video_->importBuffers(bufferCount);

Eeek. Sorry - this might be wrong here.

We're now letting applications allocate cfg.bufferCount size ... isn't
that what should be passed in here ?

This impacts the v4l2 buffer cache handling - where we track dma bufs to
v4l2_buf objects.

It would still work - but I think we'll be cycling through permanantly
hitting cache flush/misses now with this if the application sets
cfg->bufferCount > kPipelineDepth.


The allocation that happens in this call is still small - and I still
think we should be 'importing' the maximum number V4L2 lets us in most
cases - the existing v4l2 buffer cache we have would then manage these
accordingly.

I think for the time being - this series could instead pass in the
cfg.Buffer count to the two paths... perhaps with a todo to suggest that
it could easily just be the maximum allowed by v4l2 instead.

--
Kieran


>         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.48.1
>