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

23842 diff mbox series

The StreamConfiguration::bufferCount is reset to a hardcoded value of 4
in RkISP1Path::validate(). Keep the minimum value of 4 but do not reset
it, if it was set to a larger value.  This allows the user to set
bufferCount to an arbitrary number of buffers which then can be
allocated for example by the FrameBufferAllocator. If the bufferCount is
set to a smaller value it gets reset to 4 again and the configuation is
market as adjusted.

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

---

Changes in v3:
- Introduced a new constant kRkISP1MinBufferCount
- Ensure that bufferCount is at least 4 in validate
- Use the stream bufferCount for the number of import buffers, so that
  we don't issue cache flushes due to more buffers circulating than
imported in V4L2

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

Quoting Stefan Klug (2025-07-17 13:59:24)
> The StreamConfiguration::bufferCount is reset to a hardcoded value of 4
> in RkISP1Path::validate(). Keep the minimum value of 4 but do not reset
> it, if it was set to a larger value.  This allows the user to set
> bufferCount to an arbitrary number of buffers which then can be
> allocated for example by the FrameBufferAllocator. If the bufferCount is
> set to a smaller value it gets reset to 4 again and the configuation is
> market as adjusted.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Tested-By: Sven Püschel<s.pueschel@pengutronix.de>
> 
> ---
> 
> Changes in v3:
> - Introduced a new constant kRkISP1MinBufferCount
> - Ensure that bufferCount is at least 4 in validate
> - Use the stream bufferCount for the number of import buffers, so that
>   we don't issue cache flushes due to more buffers circulating than
> imported in V4L2

Thanks.


> 
> Changes in v1:
> - Removed todo comment that was solved by this change
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 13 +++++++++++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  7 ++-----
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 +---
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7954ea82fd0d..aee267a90f4b 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -164,6 +164,8 @@ namespace {
>   */
>  static constexpr unsigned int kRkISP1MaxQueuedRequests = 4;
>  
> +static constexpr unsigned int kRkISP1MinBufferCount = 4;
> +
>  } // namespace
>  
>  class PipelineHandlerRkISP1 : public PipelineHandler
> @@ -607,6 +609,12 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>                                 return false;
>                 }
>  
> +               if (tryCfg.bufferCount < kRkISP1MinBufferCount) {
> +                       tryCfg.bufferCount = kRkISP1MinBufferCount;
> +                       if (expectedStatus == Valid)
> +                               return false;
> +               }
> +
>                 cfg = tryCfg;
>                 cfg.setStream(stream);
>                 return true;
> @@ -796,6 +804,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>                         return nullptr;
>  
>                 cfg.colorSpace = colorSpace;
> +               cfg.bufferCount = kRkISP1MinBufferCount;
>                 config->addConfiguration(cfg);
>         }
>  
> @@ -1129,14 +1138,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>         }
>  
>         if (data->mainPath_->isEnabled()) {
> -               ret = mainPath_.start();
> +               ret = mainPath_.start(data->mainPathStream_.configuration().bufferCount);
>                 if (ret)
>                         return ret;
>                 actions += [&]() { mainPath_.stop(); };
>         }
>  
>         if (hasSelfPath_ && data->selfPath_->isEnabled()) {
> -               ret = selfPath_.start();
> +               ret = selfPath_.start(data->selfPathStream_.configuration().bufferCount);
>                 if (ret)
>                         return ret;

Great, that solves my concerns.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         }
> 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);
>         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
>