Message ID | 20200606150436.1851700-11-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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@ragnatech.se> > Reviewed-by: Jacopo Mondi <jacopo@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); > } > > /* -----------------------------------------------------------------------------
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; + } + } +} + 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); } /* -----------------------------------------------------------------------------