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

[Jacopo Mondi]

Hi Kieran

On Tue, May 27, 2025 at 04:39:38PM +0100, Kieran Bingham wrote:
> 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 at 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.
>

videobuf2 defines

#define VB2_MAX_FRAME	(32)

but drivers can override it (I only see the Hantro driver actually
overriding that value, but other drivers could do that in future).

Drivers using the CREATE_BUF ioctl can report the max number of
buffers, but drivers using REQBUFS don't as far as I can tell.

All of this to say that we should probably set the max number of
buffers in the pipeline and not automatically in the V4L2VideoDevice
class.

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