[v1,4/6] pipeline: rkisp1: Properly handle the bufferCount set in the stream configuration
diff mbox series

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

Commit Message

Stefan Klug June 30, 2025, 8:11 a.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>

---

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

Comments

Umang Jain June 30, 2025, 3:18 p.m. UTC | #1 
On Mon, Jun 30, 2025 at 10:11:19AM +0200, 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>
> 
> ---
> 
> 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 bd14ab237064..9d7b05490af6 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);

Question: how passing bufferCount=maxQueuedRequestsDevice_ makes
this user configurable? 

>  	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
>
Stefan Klug July 1, 2025, 12:49 p.m. UTC | #2 
Hi Umang,

Quoting Umang Jain (2025-06-30 17:18:12)
> On Mon, Jun 30, 2025 at 10:11:19AM +0200, 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>
> > 
> > ---
> > 
> > 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 bd14ab237064..9d7b05490af6 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);
> 
> Question: how passing bufferCount=maxQueuedRequestsDevice_ makes
> this user configurable? 

I'll improve patch message in the next iteration. The internal buffer
count is still fixed to 4 as before. By not modifying cfg->bufferCount,
the user can set that on the StreamConfiguration and the
FrameBufferAllocator will allocate that many buffers. (The target of the
series was not mainly to increase the number of buffers inside libcamera
but the number of buffers circling).

Best regards,
Stefan

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

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index bd14ab237064..9d7b05490af6 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);
 	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_;