[libcamera-devel] [PATCH 05/11] libcamera: ipu3: cio2: Return the FrameBuffer pointer used

[Kieran Bingham]

Hi Niklas,

On 05/11/2020 00:15, Niklas Söderlund wrote:
> When adding IPA plumbing to the IPU3 pipeline handler it will be needed
> to track witch raw buffer was queued to the CIO2 for a specific request.
> Currently if using an internally allocated raw buffer the FrameBuffer is
> not exposed outside the CIO2Device as it was not needed. Prepare for the
> IPA by exposing which infernal raw buffer is picked for each request,
> later changes will associate this with a Request.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++++++---
>  src/libcamera/pipeline/ipu3/cio2.h   |  2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  8 ++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index e43ec70fe3e4e0db..1c7b9676f52abdb6 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -262,7 +262,7 @@ int CIO2Device::stop()
>  	return ret;
>  }
>  
> -int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
> +FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  {
>  	FrameBuffer *buffer = rawBuffer;
>  
> @@ -270,7 +270,7 @@ int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  	if (!buffer) {
>  		if (availableBuffers_.empty()) {
>  			LOG(IPU3, Error) << "CIO2 buffer underrun";
> -			return -EINVAL;
> +			return nullptr;
>  		}
>  
>  		buffer = availableBuffers_.front();
> @@ -279,7 +279,11 @@ int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  
>  	buffer->setRequest(request);
>  
> -	return output_->queueBuffer(buffer);
> +	int ret = output_->queueBuffer(buffer);
> +	if (ret)
> +		return nullptr;
> +
> +	return buffer;
>  }
>  
>  void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index fa813a989fd21aec..e8b491a0c104a3e2 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -52,7 +52,7 @@ public:
>  	CameraSensor *sensor() { return sensor_; }
>  	const CameraSensor *sensor() const { return sensor_; }
>  
> -	int queueBuffer(Request *request, FrameBuffer *rawBuffer);
> +	FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);
>  	void tryReturnBuffer(FrameBuffer *buffer);
>  	Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; }
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index c559d160084f87e7..62e99a308a6136a7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -643,10 +643,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  	 * Queue a buffer on the CIO2, using the raw stream buffer provided in
>  	 * the request, if any, or a CIO2 internal buffer otherwise.
>  	 */
> -	FrameBuffer *rawBuffer = request->findBuffer(&data->rawStream_);
> -	error = data->cio2_.queueBuffer(request, rawBuffer);
> -	if (error)
> -		return error;
> +	FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);
> +	FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);
> +	if (!rawBuffer)
> +		return -ENOMEM;

It feels a bit opaque or odd to return a buffer from queueBuffer, but
given that one of the parameters is 'a raw buffer to use', returning
'which one actaully got used' doesn't feel that bad.

It just seems a bit odd that's all ... but I haven't seen how it's all
used yet.

I'll come back to this one I guess ;-)



>  
>  	/* Queue all buffers from the request aimed for the ImgU. */
>  	for (auto it : request->buffers()) {
> 

-- 
Regards
--
Kieran