[libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests in the case a buffer shortage

[Jacopo Mondi]

Hi Hiro,
  sorry took me a while to re-read the whole discussion on the
  previous version and put me in the same perspective as you and
  Laurent had

On Wed, Apr 21, 2021 at 03:48:46PM +0900, Hirokazu Honda wrote:
> PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a
> request when there are not sufficient buffers for the request.
> Since the request will be successful if it is queued later when
> enough buffers are available. The requests failed due to a buffer
> shortage should be stored and retried later in the FIFO order.
> This introduces the queue in IPU3CameraData to do that.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp   |  2 +-
>  src/libcamera/pipeline/ipu3/frames.cpp |  4 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp   | 73 +++++++++++++++++++-------
>  3 files changed, 56 insertions(+), 23 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 3cd777d1..8bbef174 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;
>  		}
>
> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
> index a1b014ee..2c4fe508 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;
>  	}
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 73306cea..3f311e58 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -66,6 +66,8 @@ public:
>  	void cio2BufferReady(FrameBuffer *buffer);
>  	void paramBufferReady(FrameBuffer *buffer);
>  	void statBufferReady(FrameBuffer *buffer);
> +	void queuePendingRequests();
> +	void cancelPendingRequests();
>
>  	CIO2Device cio2_;
>  	ImgUDevice *imgu_;
> @@ -84,6 +86,8 @@ public:
>
>  	std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
>
> +	std::queue<Request *> pendingRequests_;
> +
>  private:
>  	void queueFrameAction(unsigned int id,
>  			      const ipa::ipu3::IPU3Action &action);
> @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  	IPU3CameraData *data = cameraData(camera);
>  	int ret = 0;
>
> +	data->cancelPendingRequests();
> +
>  	data->ipa_->stop();
>
>  	ret |= data->imgu_->stop();
> @@ -774,33 +780,60 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  	freeBuffers(camera);
>  }
>
> -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> +void IPU3CameraData::cancelPendingRequests()
>  {
> -	IPU3CameraData *data = cameraData(camera);
> +	while (!pendingRequests_.empty()) {
> +		Request *request = pendingRequests_.front();
>
> -	IPU3Frames::Info *info = data->frameInfos_.create(request);
> -	if (!info)
> -		return -ENOBUFS;
> +		for (auto it : request->buffers()) {
> +			FrameBuffer *buffer = it.second;
> +			buffer->cancel();
> +			pipe_->completeBuffer(request, buffer);
> +		}
>
> -	/*
> -	 * Queue a buffer on the CIO2, using the raw stream buffer provided in
> -	 * the request, if any, or a CIO2 internal buffer otherwise.
> -	 */
> -	FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);
> -	FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);
> -	if (!rawBuffer) {
> -		data->frameInfos_.remove(info);
> -		return -ENOMEM;
> +		pipe_->completeRequest(request);
> +		pendingRequests_.pop();
>  	}
> +}
>
> -	info->rawBuffer = rawBuffer;
> +void IPU3CameraData::queuePendingRequests()
> +{
> +	while (!pendingRequests_.empty()) {
> +		Request *request = pendingRequests_.front();
>
> -	ipa::ipu3::IPU3Event ev;
> -	ev.op = ipa::ipu3::EventProcessControls;
> -	ev.frame = info->id;
> -	ev.controls = request->controls();
> -	data->ipa_->processEvent(ev);
> +		IPU3Frames::Info *info = frameInfos_.create(request);
> +		if (!info)
> +			break;
> +
> +		/*
> +		 * Queue a buffer on the CIO2, using the raw stream buffer
> +		 * provided in the request, if any, or a CIO2 internal buffer
> +		 * otherwise.
> +		 */
> +		FrameBuffer *reqRawBuffer = request->findBuffer(&rawStream_);
> +		FrameBuffer *rawBuffer = cio2_.queueBuffer(request, reqRawBuffer);
> +		if (!rawBuffer) {
> +			frameInfos_.remove(info);
> +			break;
> +		}
> +
> +		info->rawBuffer = rawBuffer;
>
> +		ipa::ipu3::IPU3Event ev;
> +		ev.op = ipa::ipu3::EventProcessControls;
> +		ev.frame = info->id;
> +		ev.controls = request->controls();
> +		ipa_->processEvent(ev);
> +
> +		pendingRequests_.pop();
> +	}
> +}
> +
> +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> +{
> +	IPU3CameraData *data = cameraData(camera);
> +	data->pendingRequests_.push(request);
> +	data->queuePendingRequests();

I originally wrote this comment:

----------------------------------------------------------
This makes it impossible to return errors to the pipeline handler base
class, even if those errors are not due to missing buffers but are
reported by the video device, in example at cio2_.queueBuffer() time.
I don't think that's desired, as errors would go unnonticed.

I would

int queueRequestDevice(Request *req)
{
        pending.push(req);
        while(!pending.empty()) {

                r = pending.front()

                info = frameInfo.create();
                if (!info)
                        break;

                pending.pop();
                rawB = request->findBuffer(req);
                raw = cio2.queue(rawB);
                if (!raw) {
                        frameInfo.remove(info);
                        return -ENOMEM;
                }

                info->rawBuffer = raw;

                ...
                ipa_->processEvent(ev);
        }

}


To maintain the ability to return errors from the video device
-------------------------------------------------------------

But then I noticed CIO2::queueBuffer() is pretty idiotic, it behaves
differently if the raw buffer is nullptr or not, and to work around
this it cannot return an error code to signal an actual queueing
error, but it just return nullptr which can mean either "I don't have
memory available" or "there's been an error in queueing the buffer to
the device". It's impossible to decouple the two error conditions and
too much magic is happening in that function which makes it not easy
predictable from the caller.

So, I would start by splitting the buffer retrieval from the CIO2 from
the buffer queueing as in:

        raw = request->findBuffer(req);
        if (!raw)
                raw = cio2->getRawBuffer();

        if (!raw)
                break;

        ret = cio2->queueBuffer(raw);
        if (ret)
                return ret;

But as you don't have to solve all the issues you encounter, the patch
is fine as it is with a little nit that I find

void funcA()
{
        while(something) {
                code;
        }
}

void funcB()
{
        funcA();
}

worse to read compared to unfolding the while loop in the caller.


>  	return 0;
>  }
>
> --
> 2.31.1.368.gbe11c130af-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel