[libcamera-devel,v9,06/18] libcamera: pipeline: simple: Don't rely on bufferCount
diff mbox series

Message ID 20221216122939.256534-7-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • lc-compliance: Add test to queue more requests than hardware depth
Related show

Commit Message

Paul Elder Dec. 16, 2022, 12:29 p.m. UTC 
From: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Currently the simple pipeline handler relies on bufferCount to decide on
the number of buffers to allocate internally when a converter is in use
and for the number of V4L2 buffer slots to reserve. Instead, the number
of internal buffers should be the minimum required by the pipeline to
keep the requests flowing, in order to avoid wasting memory, while the
number of V4L2 buffer slots should be greater than the expected number
of requests queued by the application, in order to avoid thrashing
dmabuf mappings, which would degrade performance.

Stop relying on bufferCount for these numbers and instead set them to
appropriate, and independent, constants.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v9:
- rebased

Changes in v8:
- New
---
 include/libcamera/internal/converter.h        |  3 ++-
 .../internal/converter/converter_v4l2_m2m.h   |  6 +++--
 .../converter/converter_v4l2_m2m.cpp          | 12 +++++-----
 src/libcamera/pipeline/simple/simple.cpp      | 22 ++++++++++++++-----
 4 files changed, 30 insertions(+), 13 deletions(-)

Comments

Jacopo Mondi Dec. 16, 2022, 3:17 p.m. UTC | #1 
Hi Paul

On Fri, Dec 16, 2022 at 09:29:27PM +0900, Paul Elder via libcamera-devel wrote:
> From: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>
> Currently the simple pipeline handler relies on bufferCount to decide on
> the number of buffers to allocate internally when a converter is in use
> and for the number of V4L2 buffer slots to reserve. Instead, the number
> of internal buffers should be the minimum required by the pipeline to
> keep the requests flowing, in order to avoid wasting memory, while the
> number of V4L2 buffer slots should be greater than the expected number
> of requests queued by the application, in order to avoid thrashing
> dmabuf mappings, which would degrade performance.
>
> Stop relying on bufferCount for these numbers and instead set them to
> appropriate, and independent, constants.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v9:
> - rebased
>
> Changes in v8:
> - New
> ---
>  include/libcamera/internal/converter.h        |  3 ++-
>  .../internal/converter/converter_v4l2_m2m.h   |  6 +++--
>  .../converter/converter_v4l2_m2m.cpp          | 12 +++++-----
>  src/libcamera/pipeline/simple/simple.cpp      | 22 ++++++++++++++-----
>  4 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 834ec5ab..32490fe0 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -49,7 +49,8 @@ public:
>  	virtual int exportBuffers(unsigned int output, unsigned int count,
>  				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
>
> -	virtual int start() = 0;
> +	virtual int start(unsigned int internalBufferCount,
> +			  unsigned int bufferSlotCount) = 0;
>  	virtual void stop() = 0;
>
>  	virtual int queueBuffers(FrameBuffer *input,
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 815916d0..1f471071 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -50,7 +50,8 @@ public:
>  	int exportBuffers(unsigned int ouput, unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> -	int start();
> +	int start(unsigned int internalBufferCount,
> +		  unsigned int bufferSlotCount);
>  	void stop();
>
>  	int queueBuffers(FrameBuffer *input,
> @@ -69,7 +70,8 @@ private:
>  		int exportBuffers(unsigned int count,
>  				  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> -		int start();
> +		int start(unsigned int internalBufferCount,
> +			  unsigned int bufferSlotCount);
>  		void stop();
>
>  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 2a4d1d99..9d25f25a 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -107,13 +107,14 @@ int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,
>  	return m2m_->capture()->exportBuffers(count, buffers);
>  }
>
> -int V4L2M2MConverter::Stream::start()
> +int V4L2M2MConverter::Stream::start(unsigned int internalBufferCount,
> +				    unsigned int bufferSlotCount)
>  {
> -	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> +	int ret = m2m_->output()->importBuffers(internalBufferCount);
>  	if (ret < 0)
>  		return ret;
>
> -	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> +	ret = m2m_->capture()->importBuffers(bufferSlotCount);

Are inputBufferCount_ and outputBufferCount_ used anymore ?
And to be honest I would keep the names as they are

Also the number of output buffers could be defined by the converter
class itself ?

>  	if (ret < 0) {
>  		stop();
>  		return ret;
> @@ -373,12 +374,13 @@ int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,
>  /**
>   * \copydoc libcamera::Converter::start
>   */
> -int V4L2M2MConverter::start()
> +int V4L2M2MConverter::start(unsigned int internalBufferCount,
> +			    unsigned int bufferSlotCount)
>  {
>  	int ret;
>
>  	for (Stream &stream : streams_) {
> -		ret = stream.start();
> +		ret = stream.start(internalBufferCount, bufferSlotCount);
>  		if (ret < 0) {
>  			stop();
>  			return ret;
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6b7c6d5c..196e5252 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -339,6 +339,7 @@ protected:
>
>  private:
>  	static constexpr unsigned int kNumInternalBuffers = 3;
> +	static constexpr unsigned int kSimpleBufferSlotCount = 16;
>
>  	struct EntityData {
>  		std::unique_ptr<V4L2VideoDevice> video;
> @@ -1232,17 +1233,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  		return -EBUSY;
>  	}
>
> +	/*
> +	 * Number of internal buffers that will be used to move video capture
> +	 * device frames to the converter.
> +	 *
> +	 * \todo Make this depend on the driver in use instead of being
> +	 * hardcoded. In order to not drop frames, the realtime requirements for
> +	 * each device should be checked, so it may not be as simple as just
> +	 * using the minimum number of buffers required by the driver.
> +	 */
> +	static constexpr unsigned int internalBufferCount = 3;
> +
>  	if (data->useConverter_) {
>  		/*
>  		 * When using the converter allocate a fixed number of internal
>  		 * buffers.
>  		 */
> -		ret = video->allocateBuffers(kNumInternalBuffers,
> +		ret = video->allocateBuffers(internalBufferCount,
>  					     &data->converterBuffers_);
>  	} else {
> -		/* Otherwise, prepare for using buffers from the only stream. */
> -		Stream *stream = &data->streams_[0];
> -		ret = video->importBuffers(stream->configuration().bufferCount);
> +		/* Otherwise, prepare for using buffers. */
> +		ret = video->importBuffers(kSimpleBufferSlotCount);
>  	}
>  	if (ret < 0) {
>  		releasePipeline(data);
> @@ -1258,7 +1269,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  	}
>
>  	if (data->useConverter_) {
> -		ret = data->converter_->start();
> +		ret = data->converter_->start(internalBufferCount,
> +					      kSimpleBufferSlotCount);
>  		if (ret < 0) {
>  			stop(camera);
>  			return ret;
> --
> 2.35.1
>
Paul Elder Dec. 23, 2022, 10:37 p.m. UTC | #2 
Hi Jacopo,

On Fri, Dec 16, 2022 at 04:17:05PM +0100, Jacopo Mondi wrote:
> Hi Paul
> 
> On Fri, Dec 16, 2022 at 09:29:27PM +0900, Paul Elder via libcamera-devel wrote:
> > From: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> >
> > Currently the simple pipeline handler relies on bufferCount to decide on
> > the number of buffers to allocate internally when a converter is in use
> > and for the number of V4L2 buffer slots to reserve. Instead, the number
> > of internal buffers should be the minimum required by the pipeline to
> > keep the requests flowing, in order to avoid wasting memory, while the
> > number of V4L2 buffer slots should be greater than the expected number
> > of requests queued by the application, in order to avoid thrashing
> > dmabuf mappings, which would degrade performance.
> >
> > Stop relying on bufferCount for these numbers and instead set them to
> > appropriate, and independent, constants.
> >
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > Changes in v9:
> > - rebased
> >
> > Changes in v8:
> > - New
> > ---
> >  include/libcamera/internal/converter.h        |  3 ++-
> >  .../internal/converter/converter_v4l2_m2m.h   |  6 +++--
> >  .../converter/converter_v4l2_m2m.cpp          | 12 +++++-----
> >  src/libcamera/pipeline/simple/simple.cpp      | 22 ++++++++++++++-----
> >  4 files changed, 30 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > index 834ec5ab..32490fe0 100644
> > --- a/include/libcamera/internal/converter.h
> > +++ b/include/libcamera/internal/converter.h
> > @@ -49,7 +49,8 @@ public:
> >  	virtual int exportBuffers(unsigned int output, unsigned int count,
> >  				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> >
> > -	virtual int start() = 0;
> > +	virtual int start(unsigned int internalBufferCount,
> > +			  unsigned int bufferSlotCount) = 0;
> >  	virtual void stop() = 0;
> >
> >  	virtual int queueBuffers(FrameBuffer *input,
> > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > index 815916d0..1f471071 100644
> > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > @@ -50,7 +50,8 @@ public:
> >  	int exportBuffers(unsigned int ouput, unsigned int count,
> >  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >
> > -	int start();
> > +	int start(unsigned int internalBufferCount,
> > +		  unsigned int bufferSlotCount);
> >  	void stop();
> >
> >  	int queueBuffers(FrameBuffer *input,
> > @@ -69,7 +70,8 @@ private:
> >  		int exportBuffers(unsigned int count,
> >  				  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >
> > -		int start();
> > +		int start(unsigned int internalBufferCount,
> > +			  unsigned int bufferSlotCount);
> >  		void stop();
> >
> >  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > index 2a4d1d99..9d25f25a 100644
> > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > @@ -107,13 +107,14 @@ int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,
> >  	return m2m_->capture()->exportBuffers(count, buffers);
> >  }
> >
> > -int V4L2M2MConverter::Stream::start()
> > +int V4L2M2MConverter::Stream::start(unsigned int internalBufferCount,
> > +				    unsigned int bufferSlotCount)
> >  {
> > -	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> > +	int ret = m2m_->output()->importBuffers(internalBufferCount);
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> > +	ret = m2m_->capture()->importBuffers(bufferSlotCount);
> 
> Are inputBufferCount_ and outputBufferCount_ used anymore ?

No they're not.

> And to be honest I would keep the names as they are

Hm, yeah, good point.

> 
> Also the number of output buffers could be defined by the converter
> class itself ?

Good idea.


Paul

> 
> >  	if (ret < 0) {
> >  		stop();
> >  		return ret;
> > @@ -373,12 +374,13 @@ int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,
> >  /**
> >   * \copydoc libcamera::Converter::start
> >   */
> > -int V4L2M2MConverter::start()
> > +int V4L2M2MConverter::start(unsigned int internalBufferCount,
> > +			    unsigned int bufferSlotCount)
> >  {
> >  	int ret;
> >
> >  	for (Stream &stream : streams_) {
> > -		ret = stream.start();
> > +		ret = stream.start(internalBufferCount, bufferSlotCount);
> >  		if (ret < 0) {
> >  			stop();
> >  			return ret;
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 6b7c6d5c..196e5252 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -339,6 +339,7 @@ protected:
> >
> >  private:
> >  	static constexpr unsigned int kNumInternalBuffers = 3;
> > +	static constexpr unsigned int kSimpleBufferSlotCount = 16;
> >
> >  	struct EntityData {
> >  		std::unique_ptr<V4L2VideoDevice> video;
> > @@ -1232,17 +1233,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> >  		return -EBUSY;
> >  	}
> >
> > +	/*
> > +	 * Number of internal buffers that will be used to move video capture
> > +	 * device frames to the converter.
> > +	 *
> > +	 * \todo Make this depend on the driver in use instead of being
> > +	 * hardcoded. In order to not drop frames, the realtime requirements for
> > +	 * each device should be checked, so it may not be as simple as just
> > +	 * using the minimum number of buffers required by the driver.
> > +	 */
> > +	static constexpr unsigned int internalBufferCount = 3;
> > +
> >  	if (data->useConverter_) {
> >  		/*
> >  		 * When using the converter allocate a fixed number of internal
> >  		 * buffers.
> >  		 */
> > -		ret = video->allocateBuffers(kNumInternalBuffers,
> > +		ret = video->allocateBuffers(internalBufferCount,
> >  					     &data->converterBuffers_);
> >  	} else {
> > -		/* Otherwise, prepare for using buffers from the only stream. */
> > -		Stream *stream = &data->streams_[0];
> > -		ret = video->importBuffers(stream->configuration().bufferCount);
> > +		/* Otherwise, prepare for using buffers. */
> > +		ret = video->importBuffers(kSimpleBufferSlotCount);
> >  	}
> >  	if (ret < 0) {
> >  		releasePipeline(data);
> > @@ -1258,7 +1269,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> >  	}
> >
> >  	if (data->useConverter_) {
> > -		ret = data->converter_->start();
> > +		ret = data->converter_->start(internalBufferCount,
> > +					      kSimpleBufferSlotCount);
> >  		if (ret < 0) {
> >  			stop(camera);
> >  			return ret;
> > --
> > 2.35.1
> >

Patch
diff mbox series 

diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
index 834ec5ab..32490fe0 100644
--- a/include/libcamera/internal/converter.h
+++ b/include/libcamera/internal/converter.h
@@ -49,7 +49,8 @@  public:
 	virtual int exportBuffers(unsigned int output, unsigned int count,
 				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
 
-	virtual int start() = 0;
+	virtual int start(unsigned int internalBufferCount,
+			  unsigned int bufferSlotCount) = 0;
 	virtual void stop() = 0;
 
 	virtual int queueBuffers(FrameBuffer *input,
diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
index 815916d0..1f471071 100644
--- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
+++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
@@ -50,7 +50,8 @@  public:
 	int exportBuffers(unsigned int ouput, unsigned int count,
 			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
 
-	int start();
+	int start(unsigned int internalBufferCount,
+		  unsigned int bufferSlotCount);
 	void stop();
 
 	int queueBuffers(FrameBuffer *input,
@@ -69,7 +70,8 @@  private:
 		int exportBuffers(unsigned int count,
 				  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
 
-		int start();
+		int start(unsigned int internalBufferCount,
+			  unsigned int bufferSlotCount);
 		void stop();
 
 		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
index 2a4d1d99..9d25f25a 100644
--- a/src/libcamera/converter/converter_v4l2_m2m.cpp
+++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
@@ -107,13 +107,14 @@  int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,
 	return m2m_->capture()->exportBuffers(count, buffers);
 }
 
-int V4L2M2MConverter::Stream::start()
+int V4L2M2MConverter::Stream::start(unsigned int internalBufferCount,
+				    unsigned int bufferSlotCount)
 {
-	int ret = m2m_->output()->importBuffers(inputBufferCount_);
+	int ret = m2m_->output()->importBuffers(internalBufferCount);
 	if (ret < 0)
 		return ret;
 
-	ret = m2m_->capture()->importBuffers(outputBufferCount_);
+	ret = m2m_->capture()->importBuffers(bufferSlotCount);
 	if (ret < 0) {
 		stop();
 		return ret;
@@ -373,12 +374,13 @@  int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,
 /**
  * \copydoc libcamera::Converter::start
  */
-int V4L2M2MConverter::start()
+int V4L2M2MConverter::start(unsigned int internalBufferCount,
+			    unsigned int bufferSlotCount)
 {
 	int ret;
 
 	for (Stream &stream : streams_) {
-		ret = stream.start();
+		ret = stream.start(internalBufferCount, bufferSlotCount);
 		if (ret < 0) {
 			stop();
 			return ret;
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 6b7c6d5c..196e5252 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -339,6 +339,7 @@  protected:
 
 private:
 	static constexpr unsigned int kNumInternalBuffers = 3;
+	static constexpr unsigned int kSimpleBufferSlotCount = 16;
 
 	struct EntityData {
 		std::unique_ptr<V4L2VideoDevice> video;
@@ -1232,17 +1233,27 @@  int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
 		return -EBUSY;
 	}
 
+	/*
+	 * Number of internal buffers that will be used to move video capture
+	 * device frames to the converter.
+	 *
+	 * \todo Make this depend on the driver in use instead of being
+	 * hardcoded. In order to not drop frames, the realtime requirements for
+	 * each device should be checked, so it may not be as simple as just
+	 * using the minimum number of buffers required by the driver.
+	 */
+	static constexpr unsigned int internalBufferCount = 3;
+
 	if (data->useConverter_) {
 		/*
 		 * When using the converter allocate a fixed number of internal
 		 * buffers.
 		 */
-		ret = video->allocateBuffers(kNumInternalBuffers,
+		ret = video->allocateBuffers(internalBufferCount,
 					     &data->converterBuffers_);
 	} else {
-		/* Otherwise, prepare for using buffers from the only stream. */
-		Stream *stream = &data->streams_[0];
-		ret = video->importBuffers(stream->configuration().bufferCount);
+		/* Otherwise, prepare for using buffers. */
+		ret = video->importBuffers(kSimpleBufferSlotCount);
 	}
 	if (ret < 0) {
 		releasePipeline(data);
@@ -1258,7 +1269,8 @@  int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
 	}
 
 	if (data->useConverter_) {
-		ret = data->converter_->start();
+		ret = data->converter_->start(internalBufferCount,
+					      kSimpleBufferSlotCount);
 		if (ret < 0) {
 			stop(camera);
 			return ret;