Message ID | 20200324155145.3896183-7-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, Thank you for the patch. On Tue, Mar 24, 2020 at 04:51:44PM +0100, Niklas Söderlund wrote: > Instead of unconditionally cycling buffers between the CIO2 and IMGU > pick a buffer when a request is queued to the pipeline. This is needed > if operations are to be applied to the buffer coming from CIO2 with > parameters coming from a Request. > > The approach to pick a CIO2 buffer when a request is queued is similar > to other pipelines, where parameters and statistic buffers are picked > this way. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > * Changes since RFC > - Drop cio2ToRequest_ lookup map and use Request * in FrameBuffer. > - Removed duplicated LOG() from CIO2Device::allocateBuffers. > - Reassign instead of iterate to empty availableBuffers_. > - Fold CIO2Device::queueBuffer() into the only caller. > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++------ > 1 file changed, 42 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 1b44460e4d887d14..cc602834f24108a7 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -8,6 +8,7 @@ > #include <algorithm> > #include <iomanip> > #include <memory> > +#include <queue> > #include <vector> > > #include <linux/media-bus-format.h> > @@ -118,6 +119,9 @@ public: > int allocateBuffers(); > void freeBuffers(); > > + FrameBuffer *getBuffer(); > + void putBuffer(FrameBuffer *buffer); > + > int start(); > int stop(); > > @@ -129,6 +133,7 @@ public: > > private: > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > + std::queue<FrameBuffer *> availableBuffers_; > }; > > class IPU3Stream : public Stream > @@ -716,11 +721,21 @@ 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 and track it it's used for this request. */ Sounds a bit weird. Maybe "track that it's used", or just "and associate it with the request" ? As you also queue the buffer, you may want to reflect that in the comment, "..., associate it with the request and queue it to the CIO2" ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + buffer = data->cio2_.getBuffer(); > + if (!buffer) > + return -EINVAL; > + > + buffer->setRequest(request); > + data->cio2_.output_->queueBuffer(buffer); > + > for (auto it : request->buffers()) { > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > - FrameBuffer *buffer = it.second; > + buffer = it.second; > > int ret = stream->device_->dev->queueBuffer(buffer); > if (ret < 0) > @@ -892,7 +907,7 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer) > if (buffer->metadata().status == FrameMetadata::FrameCancelled) > return; > > - cio2_.output_->queueBuffer(buffer); > + cio2_.putBuffer(buffer); > } > > /** > @@ -1416,29 +1431,45 @@ int CIO2Device::allocateBuffers() > { > int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); > if (ret < 0) > - LOG(IPU3, Error) << "Failed to allocate CIO2 buffers"; > + return ret; > + > + for (std::unique_ptr<FrameBuffer> &buffer : buffers_) > + availableBuffers_.push(buffer.get()); > > return ret; > } > > void CIO2Device::freeBuffers() > { > + availableBuffers_ = {}; > + > buffers_.clear(); > > if (output_->releaseBuffers()) > LOG(IPU3, Error) << "Failed to release CIO2 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() > { > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers_) { > - int ret = output_->queueBuffer(buffer.get()); > - if (ret) { > - LOG(IPU3, Error) << "Failed to queue CIO2 buffer"; > - return ret; > - } > - } > - > return output_->streamOn(); > } >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 1b44460e4d887d14..cc602834f24108a7 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -8,6 +8,7 @@ #include <algorithm> #include <iomanip> #include <memory> +#include <queue> #include <vector> #include <linux/media-bus-format.h> @@ -118,6 +119,9 @@ public: int allocateBuffers(); void freeBuffers(); + FrameBuffer *getBuffer(); + void putBuffer(FrameBuffer *buffer); + int start(); int stop(); @@ -129,6 +133,7 @@ public: private: std::vector<std::unique_ptr<FrameBuffer>> buffers_; + std::queue<FrameBuffer *> availableBuffers_; }; class IPU3Stream : public Stream @@ -716,11 +721,21 @@ 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 and track it it's used for this request. */ + buffer = data->cio2_.getBuffer(); + if (!buffer) + return -EINVAL; + + buffer->setRequest(request); + data->cio2_.output_->queueBuffer(buffer); + for (auto it : request->buffers()) { IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); - FrameBuffer *buffer = it.second; + buffer = it.second; int ret = stream->device_->dev->queueBuffer(buffer); if (ret < 0) @@ -892,7 +907,7 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer) if (buffer->metadata().status == FrameMetadata::FrameCancelled) return; - cio2_.output_->queueBuffer(buffer); + cio2_.putBuffer(buffer); } /** @@ -1416,29 +1431,45 @@ int CIO2Device::allocateBuffers() { int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); if (ret < 0) - LOG(IPU3, Error) << "Failed to allocate CIO2 buffers"; + return ret; + + for (std::unique_ptr<FrameBuffer> &buffer : buffers_) + availableBuffers_.push(buffer.get()); return ret; } void CIO2Device::freeBuffers() { + availableBuffers_ = {}; + buffers_.clear(); if (output_->releaseBuffers()) LOG(IPU3, Error) << "Failed to release CIO2 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() { - for (const std::unique_ptr<FrameBuffer> &buffer : buffers_) { - int ret = output_->queueBuffer(buffer.get()); - if (ret) { - LOG(IPU3, Error) << "Failed to queue CIO2 buffer"; - return ret; - } - } - return output_->streamOn(); }
Instead of unconditionally cycling buffers between the CIO2 and IMGU pick a buffer when a request is queued to the pipeline. This is needed if operations are to be applied to the buffer coming from CIO2 with parameters coming from a Request. The approach to pick a CIO2 buffer when a request is queued is similar to other pipelines, where parameters and statistic buffers are picked this way. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- * Changes since RFC - Drop cio2ToRequest_ lookup map and use Request * in FrameBuffer. - Removed duplicated LOG() from CIO2Device::allocateBuffers. - Reassign instead of iterate to empty availableBuffers_. - Fold CIO2Device::queueBuffer() into the only caller. --- src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 11 deletions(-)