[libcamera-devel,v3,10/10] libcamera: ipu3: Allow zero-copy RAW stream capture

4137 diff mbox series

With the refactored CIO2 interface it's now easy to add zero-copy for
buffers in the RAW stream. Use the internally allocated buffers inside
the CIO2Device if no buffer for the RAW stream is provided by the
application, or use the application-provided buffer if any.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
* Changes since v2
- Add todo of potential common need code.

* Changes since v1
- Update comments.
- Use Request::findBuffer() instead of own logic.
---
 src/libcamera/pipeline/ipu3/cio2.cpp | 58 ++++++++++++++---------
 src/libcamera/pipeline/ipu3/cio2.h   |  7 ++-
 src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++-------------------
 3 files changed, 61 insertions(+), 73 deletions(-)

Hi Niklas,

Thank you for the patch.

On Tue, Jun 23, 2020 at 04:09:30AM +0200, Niklas Söderlund wrote:
> With the refactored CIO2 interface it's now easy to add zero-copy for
> buffers in the RAW stream. Use the internally allocated buffers inside
> the CIO2Device if no buffer for the RAW stream is provided by the
> application, or use the application-provided buffer if any.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> * Changes since v2
> - Add todo of potential common need code.
> 
> * Changes since v1
> - Update comments.
> - Use Request::findBuffer() instead of own logic.
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 58 ++++++++++++++---------
>  src/libcamera/pipeline/ipu3/cio2.h   |  7 ++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++-------------------
>  3 files changed, 61 insertions(+), 73 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index efaa460b246697a6..ab7f96dad19b0560 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -213,31 +213,16 @@ int CIO2Device::exportBuffers(unsigned int count,
>  	return output_->exportBuffers(count, buffers);
>  }
>  
> -FrameBuffer *CIO2Device::getBuffer()
> -{
> -	if (availableBuffers_.empty()) {
> -		LOG(IPU3, Error) << "CIO2 buffer underrun";
> -		return nullptr;
> -	}
> -
> -	FrameBuffer *buffer = availableBuffers_.front();
> -
> -	availableBuffers_.pop();
> -
> -	return buffer;
> -}
> -
> -void CIO2Device::putBuffer(FrameBuffer *buffer)
> -{
> -	availableBuffers_.push(buffer);
> -}
> -
>  int CIO2Device::start()
>  {
> -	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
> +	int ret = output_->exportBuffers(CIO2_BUFFER_COUNT, &buffers_);
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = output_->importBuffers(CIO2_BUFFER_COUNT);
> +	if (ret)
> +		LOG(IPU3, Error) << "Failed to import CIO2 buffers";
> +
>  	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
>  		availableBuffers_.push(buffer.get());
>  
> @@ -269,11 +254,42 @@ int CIO2Device::stop()
>  	return ret;
>  }
>  
> -int CIO2Device::queueBuffer(FrameBuffer *buffer)
> +int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  {
> +	FrameBuffer *buffer = rawBuffer;
> +
> +	/* If no buffer is provided in the request, use an internal one. */
> +	if (!buffer) {
> +		if (availableBuffers_.empty()) {
> +			LOG(IPU3, Error) << "CIO2 buffer underrun";
> +			return -EINVAL;
> +		}
> +
> +		buffer = availableBuffers_.front();
> +		availableBuffers_.pop();
> +	}
> +
> +	buffer->setRequest(request);
> +
>  	return output_->queueBuffer(buffer);
>  }
>  
> +void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
> +{
> +	/*
> +	 * \todo Once more pipelines deal with buffers that may be allocated
> +	 * internally or externally at the this pattern might become a common

"at the this" ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 * need. At that point this check should be moved to something clever
> +	 * in FrameBuffer.
> +	 */
> +	for (const std::unique_ptr<FrameBuffer> &buf : buffers_) {
> +		if (buf.get() == buffer) {
> +			availableBuffers_.push(buffer);
> +			break;
> +		}
> +	}
> +}
> +
>  void CIO2Device::cio2BufferReady(FrameBuffer *buffer)
>  {
>  	bufferReady.emit(buffer);
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index 405e6c03755367c4..9f2e8a98af7043eb 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -18,6 +18,7 @@ namespace libcamera {
>  class CameraSensor;
>  class FrameBuffer;
>  class MediaDevice;
> +class Request;
>  class V4L2DeviceFormat;
>  class V4L2Subdevice;
>  class V4L2VideoDevice;
> @@ -40,15 +41,13 @@ public:
>  	int exportBuffers(unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  
> -	FrameBuffer *getBuffer();
> -	void putBuffer(FrameBuffer *buffer);
> -
>  	int start();
>  	int stop();
>  
>  	CameraSensor *sensor() { return sensor_; }
>  
> -	int queueBuffer(FrameBuffer *buffer);
> +	int queueBuffer(Request *request, FrameBuffer *rawBuffer);
> +	void tryReturnBuffer(FrameBuffer *buffer);
>  	Signal<FrameBuffer *> bufferReady;
>  
>  private:
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 4818027e8db1f7a3..6c43169eb0915965 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -122,7 +122,6 @@ public:
>  	}
>  
>  	void imguOutputBufferReady(FrameBuffer *buffer);
> -	void imguInputBufferReady(FrameBuffer *buffer);
>  	void cio2BufferReady(FrameBuffer *buffer);
>  
>  	CIO2Device cio2_;
> @@ -737,25 +736,24 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	FrameBuffer *buffer;
>  	int error = 0;
>  
> -	/* Get a CIO2 buffer, associate it with the request and queue it. */
> -	buffer = data->cio2_.getBuffer();
> -	if (!buffer)
> -		return -EINVAL;
> -
> -	buffer->setRequest(request);
> -	data->cio2_.queueBuffer(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 *rawBuffer = request->findBuffer(&data->rawStream_);
> +	error = data->cio2_.queueBuffer(request, rawBuffer);
> +	if (error)
> +		return error;
>  
> +	/* Queue all buffers from the request aimed for the ImgU. */
>  	for (auto it : request->buffers()) {
>  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> -		buffer = it.second;
> -
> -		/* Skip raw streams, they are copied from the CIO2 buffer. */
>  		if (stream->raw_)
>  			continue;
>  
> +		FrameBuffer *buffer = it.second;
>  		int ret = stream->device_->dev->queueBuffer(buffer);
>  		if (ret < 0)
>  			error = ret;
> @@ -885,8 +883,8 @@ int PipelineHandlerIPU3::registerCameras()
>  		 */
>  		data->cio2_.bufferReady.connect(data.get(),
>  					&IPU3CameraData::cio2BufferReady);
> -		data->imgu_->input_->bufferReady.connect(data.get(),
> -					&IPU3CameraData::imguInputBufferReady);
> +		data->imgu_->input_->bufferReady.connect(&data->cio2_,
> +					&CIO2Device::tryReturnBuffer);
>  		data->imgu_->output_.dev->bufferReady.connect(data.get(),
>  					&IPU3CameraData::imguOutputBufferReady);
>  		data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),
> @@ -915,22 +913,6 @@ int PipelineHandlerIPU3::registerCameras()
>   * Buffer Ready slots
>   */
>  
> -/**
> - * \brief Handle buffers completion at the ImgU input
> - * \param[in] buffer The completed buffer
> - *
> - * Buffers completed from the ImgU input are immediately queued back to the
> - * CIO2 unit to continue frame capture.
> - */
> -void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)
> -{
> -	/* \todo Handle buffer failures when state is set to BufferError. */
> -	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> -		return;
> -
> -	cio2_.putBuffer(buffer);
> -}
> -
>  /**
>   * \brief Handle buffers completion at the ImgU output
>   * \param[in] buffer The completed buffer
> @@ -963,27 +945,18 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>  		return;
>  
>  	Request *request = buffer->request();
> -	FrameBuffer *raw = request->findBuffer(&rawStream_);
>  
> -	if (!raw) {
> -		/* No RAW buffers present, just queue to IMGU. */
> -		imgu_->input_->queueBuffer(buffer);
> -		return;
> -	}
> -
> -	/* RAW buffers present, special care is needed. */
> -	if (request->buffers().size() > 1)
> -		imgu_->input_->queueBuffer(buffer);
> -
> -	if (raw->copyFrom(buffer))
> -		LOG(IPU3, Debug) << "Copy of FrameBuffer failed";
> -
> -	pipe_->completeBuffer(camera_, request, raw);
> -
> -	if (request->buffers().size() == 1) {
> -		cio2_.putBuffer(buffer);
> +	/*
> +	 * If the request contains a buffer for the RAW stream only, complete it
> +	 * now as there's no need for ImgU processing.
> +	 */
> +	if (request->findBuffer(&rawStream_) &&
> +	    pipe_->completeBuffer(camera_, request, buffer)) {
>  		pipe_->completeRequest(camera_, request);
> +		return;
>  	}
> +
> +	imgu_->input_->queueBuffer(buffer);
>  }
>  
>  /* -----------------------------------------------------------------------------