[libcamera-devel,RFC,v3,2/3] libcamera: ipu3: Try queuing pending requests if a buffer is available
diff mbox series

Message ID 20210408085101.1691729-3-hiroh@chromium.org
State Changes Requested
Headers show
Series
  • ipu3: Enable to handle a number of concurrent requests
Related show

Commit Message

Hirokazu Honda April 8, 2021, 8:51 a.m. UTC 
IPU3CameraData stores requests that have been failed due to a
buffer shortage. The requests should be retried once enough
buffers are available. This sets the retry function as signal to
CIO2Device and IPU3Frame, and invokes it from
CIO2Device::tryReturnBuffer() and IPU3Frame::remove().

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/libcamera/pipeline/ipu3/cio2.cpp   | 4 +++-
 src/libcamera/pipeline/ipu3/cio2.h     | 3 +++
 src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++--
 src/libcamera/pipeline/ipu3/frames.h   | 5 +++++
 src/libcamera/pipeline/ipu3/ipu3.cpp   | 4 ++++
 5 files changed, 19 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart April 20, 2021, 2:47 a.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Thu, Apr 08, 2021 at 05:51:00PM +0900, Hirokazu Honda wrote:
> IPU3CameraData stores requests that have been failed due to a
> buffer shortage. The requests should be retried once enough
> buffers are available. This sets the retry function as signal to
> CIO2Device and IPU3Frame, and invokes it from
> CIO2Device::tryReturnBuffer() and IPU3Frame::remove().
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp   | 4 +++-
>  src/libcamera/pipeline/ipu3/cio2.h     | 3 +++
>  src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++--
>  src/libcamera/pipeline/ipu3/frames.h   | 5 +++++
>  src/libcamera/pipeline/ipu3/ipu3.cpp   | 4 ++++
>  5 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 3cd777d1..6490fd46 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  	/* If no buffer is provided in the request, use an internal one. */
>  	if (!buffer) {
>  		if (availableBuffers_.empty()) {
> -			LOG(IPU3, Error) << "CIO2 buffer underrun";
> +			LOG(IPU3, Debug) << "CIO2 buffer underrun";

This belongs to 1/3. Same for the two similar changes below.

>  			return nullptr;
>  		}
>  
> @@ -302,6 +302,8 @@ void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
>  			break;
>  		}
>  	}
> +
> +	bufferAvailable_.emit();
>  }
>  
>  void CIO2Device::freeBuffers()
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index 5ecc4f47..8f37ee35 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -54,6 +54,7 @@ public:
>  	FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);
>  	void tryReturnBuffer(FrameBuffer *buffer);
>  	Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; }
> +	Signal<> &bufferAvailable() { return bufferAvailable_; }

You can declare the signal publicly directly, with

	Signal <> bufferAvailable;

The reason why the bufferReady() and frameStart() wrappers exist is to
proxy the signals from the signal from the csi2_ (V4L2Subdevice), but
for bufferAvailable, we create the signal directly in this class.

(On a side note, I've been thinking about making all signals private,
with public accessor functions, but it's because I'm used to the Qt
syntax ;-) So I'm not sure it would be useful, beyond the fact that it
would remove a violation of our coding style that makes all member
variable names end with _, which we don't follow for signals. If anyone
has any opinion, it's one of those bikeshedding discussions that we all
love.)

>  	Signal<uint32_t> &frameStart() { return csi2_->frameStart; }
>  
>  private:
> @@ -65,6 +66,8 @@ private:
>  	std::unique_ptr<V4L2Subdevice> csi2_;
>  	std::unique_ptr<V4L2VideoDevice> output_;
>  
> +	Signal<> bufferAvailable_;
> +
>  	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
>  	std::queue<FrameBuffer *> availableBuffers_;
>  };
> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
> index 03e8131c..58a47f98 100644
> --- a/src/libcamera/pipeline/ipu3/frames.cpp
> +++ b/src/libcamera/pipeline/ipu3/frames.cpp
> @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)
>  	unsigned int id = request->sequence();
>  
>  	if (availableParamBuffers_.empty()) {
> -		LOG(IPU3, Error) << "Parameters buffer underrun";
> +		LOG(IPU3, Debug) << "Parameters buffer underrun";
>  		return nullptr;
>  	}
>  
>  	if (availableStatBuffers_.empty()) {
> -		LOG(IPU3, Error) << "Statistics buffer underrun";
> +		LOG(IPU3, Debug) << "Statistics buffer underrun";
>  		return nullptr;
>  	}
>  
> @@ -86,6 +86,8 @@ void IPU3Frames::remove(IPU3Frames::Info *info)
>  
>  	/* Delete the extended frame information. */
>  	frameInfo_.erase(info->id);
> +
> +	bufferAvailable_.emit();

I think this should be emitted in IPU3Frames::tryComplete(). You call
IPU3Frames::remove() in IPU3CameraData::queuePendingRequests() in patch
1/3, which would then emit the signal, which would call back into
IPU3CameraData::queuePendingRequests(), and that's a possible infinite
loop.

>  }
>  
>  bool IPU3Frames::tryComplete(IPU3Frames::Info *info)
> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h
> index 4acdf48e..d2e5fbb9 100644
> --- a/src/libcamera/pipeline/ipu3/frames.h
> +++ b/src/libcamera/pipeline/ipu3/frames.h
> @@ -12,6 +12,8 @@
>  #include <queue>
>  #include <vector>
>  
> +#include <libcamera/signal.h>
> +
>  namespace libcamera {
>  
>  class FrameBuffer;
> @@ -49,10 +51,13 @@ public:
>  	Info *find(unsigned int id);
>  	Info *find(FrameBuffer *buffer);
>  
> +	Signal<> &bufferAvailable() { return bufferAvailable_; }

Missing blank line.

>  private:
>  	std::queue<FrameBuffer *> availableParamBuffers_;
>  	std::queue<FrameBuffer *> availableStatBuffers_;
>  
> +	Signal<> bufferAvailable_;
> +
>  	std::map<unsigned int, std::unique_ptr<Info>> frameInfo_;
>  };
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index c73e4f7c..462a0d9b 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -699,6 +699,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>  	data->ipa_->mapBuffers(ipaBuffers_);
>  
>  	data->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);
> +	data->frameInfos_.bufferAvailable().connect(
> +		data, &IPU3CameraData::queuePendingRequests);
>  
>  	return 0;
>  }
> @@ -1123,6 +1125,8 @@ int PipelineHandlerIPU3::registerCameras()
>  		 */
>  		data->cio2_.bufferReady().connect(data.get(),
>  					&IPU3CameraData::cio2BufferReady);
> +		data->cio2_.bufferAvailable().connect(
> +			data.get(), &IPU3CameraData::queuePendingRequests);
>  		data->imgu_->input_->bufferReady.connect(&data->cio2_,
>  					&CIO2Device::tryReturnBuffer);
>  		data->imgu_->output_->bufferReady.connect(data.get(),

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
index 3cd777d1..6490fd46 100644
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -272,7 +272,7 @@  FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
 	/* If no buffer is provided in the request, use an internal one. */
 	if (!buffer) {
 		if (availableBuffers_.empty()) {
-			LOG(IPU3, Error) << "CIO2 buffer underrun";
+			LOG(IPU3, Debug) << "CIO2 buffer underrun";
 			return nullptr;
 		}
 
@@ -302,6 +302,8 @@  void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
 			break;
 		}
 	}
+
+	bufferAvailable_.emit();
 }
 
 void CIO2Device::freeBuffers()
diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
index 5ecc4f47..8f37ee35 100644
--- a/src/libcamera/pipeline/ipu3/cio2.h
+++ b/src/libcamera/pipeline/ipu3/cio2.h
@@ -54,6 +54,7 @@  public:
 	FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);
 	void tryReturnBuffer(FrameBuffer *buffer);
 	Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; }
+	Signal<> &bufferAvailable() { return bufferAvailable_; }
 	Signal<uint32_t> &frameStart() { return csi2_->frameStart; }
 
 private:
@@ -65,6 +66,8 @@  private:
 	std::unique_ptr<V4L2Subdevice> csi2_;
 	std::unique_ptr<V4L2VideoDevice> output_;
 
+	Signal<> bufferAvailable_;
+
 	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
 	std::queue<FrameBuffer *> availableBuffers_;
 };
diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
index 03e8131c..58a47f98 100644
--- a/src/libcamera/pipeline/ipu3/frames.cpp
+++ b/src/libcamera/pipeline/ipu3/frames.cpp
@@ -44,12 +44,12 @@  IPU3Frames::Info *IPU3Frames::create(Request *request)
 	unsigned int id = request->sequence();
 
 	if (availableParamBuffers_.empty()) {
-		LOG(IPU3, Error) << "Parameters buffer underrun";
+		LOG(IPU3, Debug) << "Parameters buffer underrun";
 		return nullptr;
 	}
 
 	if (availableStatBuffers_.empty()) {
-		LOG(IPU3, Error) << "Statistics buffer underrun";
+		LOG(IPU3, Debug) << "Statistics buffer underrun";
 		return nullptr;
 	}
 
@@ -86,6 +86,8 @@  void IPU3Frames::remove(IPU3Frames::Info *info)
 
 	/* Delete the extended frame information. */
 	frameInfo_.erase(info->id);
+
+	bufferAvailable_.emit();
 }
 
 bool IPU3Frames::tryComplete(IPU3Frames::Info *info)
diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h
index 4acdf48e..d2e5fbb9 100644
--- a/src/libcamera/pipeline/ipu3/frames.h
+++ b/src/libcamera/pipeline/ipu3/frames.h
@@ -12,6 +12,8 @@ 
 #include <queue>
 #include <vector>
 
+#include <libcamera/signal.h>
+
 namespace libcamera {
 
 class FrameBuffer;
@@ -49,10 +51,13 @@  public:
 	Info *find(unsigned int id);
 	Info *find(FrameBuffer *buffer);
 
+	Signal<> &bufferAvailable() { return bufferAvailable_; }
 private:
 	std::queue<FrameBuffer *> availableParamBuffers_;
 	std::queue<FrameBuffer *> availableStatBuffers_;
 
+	Signal<> bufferAvailable_;
+
 	std::map<unsigned int, std::unique_ptr<Info>> frameInfo_;
 };
 
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index c73e4f7c..462a0d9b 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -699,6 +699,8 @@  int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
 	data->ipa_->mapBuffers(ipaBuffers_);
 
 	data->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);
+	data->frameInfos_.bufferAvailable().connect(
+		data, &IPU3CameraData::queuePendingRequests);
 
 	return 0;
 }
@@ -1123,6 +1125,8 @@  int PipelineHandlerIPU3::registerCameras()
 		 */
 		data->cio2_.bufferReady().connect(data.get(),
 					&IPU3CameraData::cio2BufferReady);
+		data->cio2_.bufferAvailable().connect(
+			data.get(), &IPU3CameraData::queuePendingRequests);
 		data->imgu_->input_->bufferReady.connect(&data->cio2_,
 					&CIO2Device::tryReturnBuffer);
 		data->imgu_->output_->bufferReady.connect(data.get(),