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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart