[libcamera-devel] [PATCH v9 06/18] libcamera: pipeline: simple: Don't rely on bufferCount

[Paul Elder]

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 at 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 at collabora.com>
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > Signed-off-by: Paul Elder <paul.elder at 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
> >