[libcamera-devel] [PATCH v2 10/10] libcamera: ipu3: Allow zero-copy RAW stream capture
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 6 23:59:10 CEST 2020
Hi Niklas,
Thank you for the patch.
On Sat, Jun 06, 2020 at 05:04:36PM +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 at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> * Changes since v1
> - Update comments.
> - Use Request::findBuffer() instead of own logic.
> ---
> src/libcamera/pipeline/ipu3/cio2.cpp | 52 ++++++++++++---------
> src/libcamera/pipeline/ipu3/cio2.h | 7 ++-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++-------------------
> 3 files changed, 55 insertions(+), 73 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 9298f10d8d8c07f7..753f20fb615f6748 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -235,31 +235,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());
>
> @@ -291,11 +276,36 @@ 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)
> +{
> + for (const std::unique_ptr<FrameBuffer> &buf : buffers_) {
> + if (buf.get() == buffer) {
> + availableBuffers_.push(buffer);
> + break;
> + }
> + }
I think you may have missed my comment on v1:
I believe this will be a common need. Should the FrameBuffer class be
extended with an internal (or external) bool flag ? Or a void *owner (a
bit of a hack) ? Or something similar but better ? Bonus points if the
API to do so can be hidden from applications.
This could be done on top, but should be recorded somewhere.
> +}
> +
> 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 ffb401da6b576556..9bc01b8aa92446b0 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -20,6 +20,7 @@ namespace libcamera {
> class CameraSensor;
> class FrameBuffer;
> class MediaDevice;
> +class Request;
> class V4L2DeviceFormat;
> class V4L2Subdevice;
> class V4L2VideoDevice;
> @@ -42,15 +43,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 c95c1bfbce914801..f183ec54daed6f57 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -121,7 +121,6 @@ public:
> }
>
> void imguOutputBufferReady(FrameBuffer *buffer);
> - void imguInputBufferReady(FrameBuffer *buffer);
> void cio2BufferReady(FrameBuffer *buffer);
>
> CIO2Device cio2_;
> @@ -725,25 +724,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;
> @@ -873,8 +871,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(),
> @@ -903,22 +901,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
> @@ -951,27 +933,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);
> }
>
> /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list