[libcamera-devel] [PATCH 10/10] libcamera: ipu3: Allow zero-copy RAW stream capture
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 6 00:56:44 CEST 2020
Hi Niklas,
Thank you for the patch.
On Tue, Jun 02, 2020 at 03:28:20PM +0200, Jacopo Mondi wrote:
> Hi Niklas,
>
> not my strongest experties area, but I only have two minor remarks on
> this one.
>
> On Tue, Jun 02, 2020 at 03:39:09AM +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 one if one is.
>
> s/if one is./if any./
"or use the application-provided buffer if any."
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/pipeline/ipu3/cio2.cpp | 52 +++++++++++--------
> > src/libcamera/pipeline/ipu3/cio2.h | 6 +--
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++-----------------
> > 3 files changed, 62 insertions(+), 72 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index 895848d2b3a1fba9..bbf18910c0e7abd4 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -205,31 +205,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());
> >
> > @@ -253,11 +238,36 @@ int CIO2Device::stop()
> > return ret;
> > }
> >
> > -int CIO2Device::queueBuffer(FrameBuffer *buffer)
> > +int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
> > {
> > + FrameBuffer *buffer = rawBuffer;
> > +
> > + /* If no buffer provided in request, use an internal one. */
s/provided/is provided/
s/request/the request/
> > + 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 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.
> > +}
> > +
> > V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
> > {
> > switch (code) {
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > index 032b91c082889a63..1a9b700bf4e7e15c 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > @@ -47,9 +47,6 @@ public:
> > int exportBuffers(unsigned int count,
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >
> > - FrameBuffer *getBuffer();
> > - void putBuffer(FrameBuffer *buffer);
> > -
> > int start();
> > int stop();
> >
> > @@ -57,7 +54,8 @@ public:
> > Size resolution() const { return sensor_->resolution(); }
> > ControlList properties() const { return sensor_->properties(); }
> >
> > - 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 636c1c54627b5777..03e954d0f30d39cc 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_;
> > @@ -730,25 +729,31 @@ 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;
> > + /* Search for a RAW buffer in the request, if any. */
> > + FrameBuffer *rawBuffer = nullptr;
> > + for (auto it : request->buffers()) {
> > + IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> > + if (!stream->raw_)
> > + continue;
> >
> > - buffer->setRequest(request);
> > - data->cio2_.queueBuffer(buffer);
> > + rawBuffer = it.second;
> > + break;
> > + }
Can't you use Request::findBuffer() ?
> >
> > + /* Queue a buffer on the CIO2, the buffer may come from the request. */
> > + error = data->cio2_.queueBuffer(request, rawBuffer);
How about
/*
* 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;
> > @@ -878,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(),
> > @@ -908,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;
We're losing this, but I think it's actually not needed.
> > -
> > - cio2_.putBuffer(buffer);
> > -}
> > -
> > /**
> > * \brief Handle buffers completion at the ImgU output
> > * \param[in] buffer The completed buffer
> > @@ -956,27 +945,20 @@ 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 requests contains a buffer for the RAW stream and the buffer
> > + * completed by the CIO2 satisfy all pending buffers in the request
>
> s/satisfy/satisfies
>
> The rest looks good
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > + * complete the request now, there is no need for this request to be
> > + * processed by the ImGU.
> > + */
> > + if (request->findBuffer(&rawStream_) &&
> > + pipe_->completeBuffer(camera_, request, buffer)) {
> > pipe_->completeRequest(camera_, request);
> > + return;
> > }
I find the comment a bit hard to parse. How about the following ?
/*
* If the request contains a buffer for the RAW stream only, complete it
* now as there's no need for ImgU processing.
*/
> > +
> > + imgu_->input_->queueBuffer(buffer);
> > }
> >
> > /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list