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

Message ID 20250526214224.13631-5-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • rkisp1: Allow usage of more than 4 buffers
Related show

Commit Message

Stefan Klug May 26, 2025, 9:42 p.m. UTC 
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(-)

Comments

Kieran Bingham May 27, 2025, 3:39 p.m. UTC | #1 
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
>
Sven Püschel May 30, 2025, 5:48 a.m. UTC | #2 
Hi Stefan,

just noticed a typo in the commit title Prperly -> Properly

On 5/26/25 23:42, Stefan Klug wrote:
> 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;
>   		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);

The todo can also be removed with this change.


Sincerely

     Sven

>   	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_;
>
Jacopo Mondi May 30, 2025, 9:58 a.m. UTC | #3 
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@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
> >

Patch
diff mbox series 

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