[libcamera-devel,v4,5/6] libcamera: pipeline: Don't rely on bufferCount
diff mbox series

Message ID 20210506180249.318346-6-nfraprado@collabora.com
State Superseded
Headers show
Series
  • lc-compliance: Add test to queue more requests than hardware depth
Related show

Commit Message

NĂ­colas F. R. A. Prado May 6, 2021, 6:02 p.m. UTC
Pipelines have relied on bufferCount to decide on the number of buffers
to allocate internally through allocateBuffers() and on the number of
V4L2 buffer slots to reserve through importBuffers(). Instead, the
number of internal buffers should be the minimum required by the
algorithms to avoid wasting memory, and the number of V4L2 buffer slots
should overallocate to avoid trashing dmabuf mappings.

For now, just set them to constants and stop relying on bufferCount, to
allow for its removal.

Signed-off-by: NĂ­colas F. R. A. Prado <nfraprado@collabora.com>
---
 src/libcamera/pipeline/ipu3/imgu.cpp              | 12 ++++++------
 src/libcamera/pipeline/ipu3/imgu.h                |  5 ++++-
 src/libcamera/pipeline/ipu3/ipu3.cpp              |  9 +--------
 .../pipeline/raspberrypi/raspberrypi.cpp          | 15 +++++----------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp          |  9 ++-------
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp     |  2 +-
 src/libcamera/pipeline/rkisp1/rkisp1_path.h       |  3 +++
 src/libcamera/pipeline/simple/converter.cpp       |  4 ++--
 src/libcamera/pipeline/simple/converter.h         |  3 +++
 src/libcamera/pipeline/simple/simple.cpp          |  6 ++----
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp      |  5 +++--
 src/libcamera/pipeline/vimc/vimc.cpp              |  5 +++--
 12 files changed, 35 insertions(+), 43 deletions(-)

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
index d5cf05b0c421..720b598d1077 100644
--- a/src/libcamera/pipeline/ipu3/imgu.cpp
+++ b/src/libcamera/pipeline/ipu3/imgu.cpp
@@ -543,22 +543,22 @@  int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
 /**
  * \brief Allocate buffers for all the ImgU video devices
  */
-int ImgUDevice::allocateBuffers(unsigned int bufferCount)
+int ImgUDevice::allocateBuffers()
 {
 	/* Share buffers between CIO2 output and ImgU input. */
-	int ret = input_->importBuffers(bufferCount);
+	int ret = input_->importBuffers(BUFFER_SLOT_COUNT);
 	if (ret) {
 		LOG(IPU3, Error) << "Failed to import ImgU input buffers";
 		return ret;
 	}
 
-	ret = param_->allocateBuffers(bufferCount, &paramBuffers_);
+	ret = param_->allocateBuffers(INTERNAL_BUFFER_COUNT, &paramBuffers_);
 	if (ret < 0) {
 		LOG(IPU3, Error) << "Failed to allocate ImgU param buffers";
 		goto error;
 	}
 
-	ret = stat_->allocateBuffers(bufferCount, &statBuffers_);
+	ret = stat_->allocateBuffers(INTERNAL_BUFFER_COUNT, &statBuffers_);
 	if (ret < 0) {
 		LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers";
 		goto error;
@@ -569,13 +569,13 @@  int ImgUDevice::allocateBuffers(unsigned int bufferCount)
 	 * corresponding stream is active or inactive, as the driver needs
 	 * buffers to be requested on the V4L2 devices in order to operate.
 	 */
-	ret = output_->importBuffers(bufferCount);
+	ret = output_->importBuffers(BUFFER_SLOT_COUNT);
 	if (ret < 0) {
 		LOG(IPU3, Error) << "Failed to import ImgU output buffers";
 		goto error;
 	}
 
-	ret = viewfinder_->importBuffers(bufferCount);
+	ret = viewfinder_->importBuffers(BUFFER_SLOT_COUNT);
 	if (ret < 0) {
 		LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers";
 		goto error;
diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
index 9d4915116087..4163a8dc0875 100644
--- a/src/libcamera/pipeline/ipu3/imgu.h
+++ b/src/libcamera/pipeline/ipu3/imgu.h
@@ -61,7 +61,7 @@  public:
 					    outputFormat);
 	}
 
-	int allocateBuffers(unsigned int bufferCount);
+	int allocateBuffers();
 	void freeBuffers();
 
 	int start();
@@ -86,6 +86,9 @@  private:
 	static constexpr unsigned int PAD_VF = 3;
 	static constexpr unsigned int PAD_STAT = 4;
 
+	static constexpr unsigned int INTERNAL_BUFFER_COUNT = 4;
+	static constexpr unsigned int BUFFER_SLOT_COUNT = 5;
+
 	int linkSetup(const std::string &source, unsigned int sourcePad,
 		      const std::string &sink, unsigned int sinkPad,
 		      bool enable);
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index d9e0d8a32afb..857cafcbbb72 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -668,16 +668,9 @@  int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
 {
 	IPU3CameraData *data = cameraData(camera);
 	ImgUDevice *imgu = data->imgu_;
-	unsigned int bufferCount;
 	int ret;
 
-	bufferCount = std::max({
-		data->outStream_.configuration().bufferCount,
-		data->vfStream_.configuration().bufferCount,
-		data->rawStream_.configuration().bufferCount,
-	});
-
-	ret = imgu->allocateBuffers(bufferCount);
+	ret = imgu->allocateBuffers();
 	if (ret < 0)
 		return ret;
 
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 56eaf77c7566..ca31e437a431 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1144,20 +1144,15 @@  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
 {
 	RPiCameraData *data = cameraData(camera);
 	int ret;
+	constexpr unsigned int bufferCount = 4;
 
 	/*
-	 * Decide how many internal buffers to allocate. For now, simply look
-	 * at how many external buffers will be provided. We'll need to improve
-	 * this logic. However, we really must have all streams allocate the same
-	 * number of buffers to simplify error handling in queueRequestDevice().
+	 * Allocate internal buffers. We really must have all streams allocate
+	 * the same number of buffers to simplify error handling in
+	 * queueRequestDevice().
 	 */
-	unsigned int maxBuffers = 0;
-	for (const Stream *s : camera->streams())
-		if (static_cast<const RPi::Stream *>(s)->isExternal())
-			maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
-
 	for (auto const stream : data->streams_) {
-		ret = stream->prepareBuffers(maxBuffers);
+		ret = stream->prepareBuffers(bufferCount);
 		if (ret < 0)
 			return ret;
 	}
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index d8cc7c2c9fb5..f09e247a9ae1 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -689,16 +689,11 @@  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
 	unsigned int ipaBufferId = 1;
 	int ret;
 
-	unsigned int maxCount = std::max({
-		data->mainPathStream_.configuration().bufferCount,
-		data->selfPathStream_.configuration().bufferCount,
-	});
-
-	ret = param_->allocateBuffers(maxCount, &paramBuffers_);
+	ret = param_->allocateBuffers(INTERNAL_BUFFER_COUNT, &paramBuffers_);
 	if (ret < 0)
 		goto error;
 
-	ret = stat_->allocateBuffers(maxCount, &statBuffers_);
+	ret = stat_->allocateBuffers(INTERNAL_BUFFER_COUNT, &statBuffers_);
 	if (ret < 0)
 		goto error;
 
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
index 25f482eb8d8e..ec281fef316b 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
@@ -172,7 +172,7 @@  int RkISP1Path::start()
 		return -EBUSY;
 
 	/* \todo Make buffer count user configurable. */
-	ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
+	ret = video_->importBuffers(BUFFER_SLOT_COUNT);
 	if (ret)
 		return ret;
 
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
index 3b3e37d258d0..2133914228aa 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
@@ -26,6 +26,9 @@  class V4L2Subdevice;
 struct StreamConfiguration;
 struct V4L2SubdeviceFormat;
 
+static constexpr unsigned int INTERNAL_BUFFER_COUNT = 4;
+static constexpr unsigned int BUFFER_SLOT_COUNT = 5;
+
 class RkISP1Path
 {
 public:
diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
index 68644ef6477f..bf491cf661eb 100644
--- a/src/libcamera/pipeline/simple/converter.cpp
+++ b/src/libcamera/pipeline/simple/converter.cpp
@@ -100,11 +100,11 @@  int SimpleConverter::Stream::exportBuffers(unsigned int count,
 
 int SimpleConverter::Stream::start()
 {
-	int ret = m2m_->output()->importBuffers(inputBufferCount_);
+	int ret = m2m_->output()->importBuffers(BUFFER_SLOT_COUNT);
 	if (ret < 0)
 		return ret;
 
-	ret = m2m_->capture()->importBuffers(outputBufferCount_);
+	ret = m2m_->capture()->importBuffers(BUFFER_SLOT_COUNT);
 	if (ret < 0) {
 		stop();
 		return ret;
diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
index 480e528d2210..6a7031b78ebf 100644
--- a/src/libcamera/pipeline/simple/converter.h
+++ b/src/libcamera/pipeline/simple/converter.h
@@ -29,6 +29,9 @@  class SizeRange;
 struct StreamConfiguration;
 class V4L2M2MDevice;
 
+constexpr unsigned int INTERNAL_BUFFER_COUNT = 5;
+constexpr unsigned int BUFFER_SLOT_COUNT = 5;
+
 class SimpleConverter
 {
 public:
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index c949efe5c422..a6e432272364 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -792,12 +792,10 @@  int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
 		 * When using the converter allocate a fixed number of internal
 		 * buffers.
 		 */
-		ret = video->allocateBuffers(kNumInternalBuffers,
+		ret = video->allocateBuffers(INTERNAL_BUFFER_COUNT,
 					     &data->converterBuffers_);
 	} else {
-		/* Otherwise, prepare for using buffers from the only stream. */
-		Stream *stream = &data->streams_[0];
-		ret = video->importBuffers(stream->configuration().bufferCount);
+		ret = video->importBuffers(BUFFER_SLOT_COUNT);
 	}
 	if (ret < 0)
 		return ret;
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 1f77629bb8fc..1052b6f2fe5f 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -91,6 +91,8 @@  private:
 		return static_cast<UVCCameraData *>(
 			PipelineHandler::cameraData(camera));
 	}
+
+	static constexpr unsigned int BUFFER_SLOT_COUNT = 5;
 };
 
 UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data)
@@ -236,9 +238,8 @@  int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,
 int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
 {
 	UVCCameraData *data = cameraData(camera);
-	unsigned int count = data->stream_.configuration().bufferCount;
 
-	int ret = data->video_->importBuffers(count);
+	int ret = data->video_->importBuffers(BUFFER_SLOT_COUNT);
 	if (ret < 0)
 		return ret;
 
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 82e520dd7162..161d8ba6368d 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -102,6 +102,8 @@  private:
 		return static_cast<VimcCameraData *>(
 			PipelineHandler::cameraData(camera));
 	}
+
+	static constexpr unsigned int BUFFER_SLOT_COUNT = 5;
 };
 
 namespace {
@@ -312,9 +314,8 @@  int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,
 int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
 {
 	VimcCameraData *data = cameraData(camera);
-	unsigned int count = data->stream_.configuration().bufferCount;
 
-	int ret = data->video_->importBuffers(count);
+	int ret = data->video_->importBuffers(BUFFER_SLOT_COUNT);
 	if (ret < 0)
 		return ret;