| Message ID | 20200316024146.2474424-6-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Mon, Mar 16, 2020 at 03:41:45AM +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 the be applied to the buffer coming from CIO2 with s/the be/to be/ > 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> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++----- > 1 file changed, 58 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 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/drm_fourcc.h> > @@ -119,6 +120,10 @@ public: > int allocateBuffers(); > void freeBuffers(); > > + FrameBuffer *getBuffer(); > + void putBuffer(FrameBuffer *buffer); > + int queueBuffer(FrameBuffer *buffer); > + > int start(); > int stop(); > > @@ -130,6 +135,7 @@ public: > > private: > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > + std::queue<FrameBuffer *> availableBuffers_; > }; > > class IPU3Stream : public Stream > @@ -157,6 +163,8 @@ public: > void imguInputBufferReady(FrameBuffer *buffer); > void cio2BufferReady(FrameBuffer *buffer); > > + std::map<FrameBuffer *, Request *> cio2ToRequest_; We have a Request pointer in FrameBuffer to track this. It's a private field, pipeline handlers can't set it, but I think it's a good time to fix that issue properly. > + > CIO2Device cio2_; > ImgUDevice *imgu_; > > @@ -717,11 +725,20 @@ 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; We can return an error for now, but we'll really have to handle this better. If the application queues more request than the pipeline handler internal depth, they need to be put in a queue somewhere. I wonder if we should try address this already. > + data->cio2ToRequest_[buffer] = request; > + data->cio2_.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) > @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer) > if (buffer->metadata().status == FrameMetadata::FrameCancelled) > return; > > - cio2_.output_->queueBuffer(buffer); > + /* Remove association between CIO2 buffer an Request. */ > + cio2ToRequest_.erase(buffer); > + cio2_.putBuffer(buffer); > } > > /** > @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size, > int CIO2Device::allocateBuffers() > { > int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); > - if (ret < 0) > + if (ret < 0) { > LOG(IPU3, Error) << "Failed to allocate CIO2 buffers"; There's already a message printed by the V4L2VideoDevice class, do we need another one here ? > + return ret; > + } > + > + for (std::unique_ptr<FrameBuffer> &buffer : buffers_) > + availableBuffers_.push(buffer.get()); > > return ret; > } > > void CIO2Device::freeBuffers() > { > + while (!availableBuffers_.empty()) > + availableBuffers_.pop(); > + How about 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::queueBuffer(FrameBuffer *buffer) > +{ > + int ret = output_->queueBuffer(buffer); > + if (ret) > + LOG(IPU3, Error) << "Failed to queue CIO2 buffer"; > + > + return ret; Do we need this wrapper, can't we call cio2_.output_->queueBuffer(buffer) in the caller like we used to ? > +} > + > 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(); > } >
Hi Laurent, Thanks for your feedback. On 2020-03-23 14:26:06 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Mon, Mar 16, 2020 at 03:41:45AM +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 the be applied to the buffer coming from CIO2 with > > s/the be/to be/ > > > 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> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++----- > > 1 file changed, 58 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 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/drm_fourcc.h> > > @@ -119,6 +120,10 @@ public: > > int allocateBuffers(); > > void freeBuffers(); > > > > + FrameBuffer *getBuffer(); > > + void putBuffer(FrameBuffer *buffer); > > + int queueBuffer(FrameBuffer *buffer); > > + > > int start(); > > int stop(); > > > > @@ -130,6 +135,7 @@ public: > > > > private: > > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > > + std::queue<FrameBuffer *> availableBuffers_; > > }; > > > > class IPU3Stream : public Stream > > @@ -157,6 +163,8 @@ public: > > void imguInputBufferReady(FrameBuffer *buffer); > > void cio2BufferReady(FrameBuffer *buffer); > > > > + std::map<FrameBuffer *, Request *> cio2ToRequest_; > > We have a Request pointer in FrameBuffer to track this. It's a private > field, pipeline handlers can't set it, but I think it's a good time to > fix that issue properly. Good idea, I will try and improve on this for v1. > > > + > > CIO2Device cio2_; > > ImgUDevice *imgu_; > > > > @@ -717,11 +725,20 @@ 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; > > We can return an error for now, but we'll really have to handle this > better. If the application queues more request than the pipeline handler > internal depth, they need to be put in a queue somewhere. I wonder if we > should try address this already. I'm all for starting work on this topic now. But do you think it really should be solved in individual pipeline handlers and not on a framework level? When I thought about this in the past I always imaged the pipeline to report its min and max pipeline depth and then having the framework doing the heavy lifting of keeping and flushing a queue. If so I think it the behavior here is correct, but if you think pipelines shall deal with this it should be reworked. > > > + data->cio2ToRequest_[buffer] = request; > > + data->cio2_.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) > > @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer) > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) > > return; > > > > - cio2_.output_->queueBuffer(buffer); > > + /* Remove association between CIO2 buffer an Request. */ > > + cio2ToRequest_.erase(buffer); > > + cio2_.putBuffer(buffer); > > } > > > > /** > > @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size, > > int CIO2Device::allocateBuffers() > > { > > int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); > > - if (ret < 0) > > + if (ret < 0) { > > LOG(IPU3, Error) << "Failed to allocate CIO2 buffers"; > > There's already a message printed by the V4L2VideoDevice class, do we > need another one here ? I'm all for dropping it ;-) > > > + return ret; > > + } > > + > > + for (std::unique_ptr<FrameBuffer> &buffer : buffers_) > > + availableBuffers_.push(buffer.get()); > > > > return ret; > > } > > > > void CIO2Device::freeBuffers() > > { > > + while (!availableBuffers_.empty()) > > + availableBuffers_.pop(); > > + > > How about > > availableBuffers_ = {}; Nice idea. > > ? > > > 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::queueBuffer(FrameBuffer *buffer) > > +{ > > + int ret = output_->queueBuffer(buffer); > > + if (ret) > > + LOG(IPU3, Error) << "Failed to queue CIO2 buffer"; > > + > > + return ret; > > Do we need this wrapper, can't we call > cio2_.output_->queueBuffer(buffer) in the caller like we used to ? Not really, I added it as I think this driver is quiet messy and breaking things apart could be a good thing. Will drop for v1 and then we can refactor on top to make it more readable. > > > +} > > + > > 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(); > > } > > > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Tue, Mar 24, 2020 at 01:33:32PM +0100, Niklas Söderlund wrote: > On 2020-03-23 14:26:06 +0200, Laurent Pinchart wrote: > > On Mon, Mar 16, 2020 at 03:41:45AM +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 the be applied to the buffer coming from CIO2 with > > > > s/the be/to be/ > > > > > 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> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++----- > > > 1 file changed, 58 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 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/drm_fourcc.h> > > > @@ -119,6 +120,10 @@ public: > > > int allocateBuffers(); > > > void freeBuffers(); > > > > > > + FrameBuffer *getBuffer(); > > > + void putBuffer(FrameBuffer *buffer); > > > + int queueBuffer(FrameBuffer *buffer); > > > + > > > int start(); > > > int stop(); > > > > > > @@ -130,6 +135,7 @@ public: > > > > > > private: > > > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > > > + std::queue<FrameBuffer *> availableBuffers_; > > > }; > > > > > > class IPU3Stream : public Stream > > > @@ -157,6 +163,8 @@ public: > > > void imguInputBufferReady(FrameBuffer *buffer); > > > void cio2BufferReady(FrameBuffer *buffer); > > > > > > + std::map<FrameBuffer *, Request *> cio2ToRequest_; > > > > We have a Request pointer in FrameBuffer to track this. It's a private > > field, pipeline handlers can't set it, but I think it's a good time to > > fix that issue properly. > > Good idea, I will try and improve on this for v1. > > > > + > > > CIO2Device cio2_; > > > ImgUDevice *imgu_; > > > > > > @@ -717,11 +725,20 @@ 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; > > > > We can return an error for now, but we'll really have to handle this > > better. If the application queues more request than the pipeline handler > > internal depth, they need to be put in a queue somewhere. I wonder if we > > should try address this already. > > I'm all for starting work on this topic now. But do you think it really > should be solved in individual pipeline handlers and not on a framework > level? > > When I thought about this in the past I always imaged the pipeline to > report its min and max pipeline depth and then having the framework > doing the heavy lifting of keeping and flushing a queue. If so I think > it the behavior here is correct, but if you think pipelines shall deal > with this it should be reworked. I think the interface between the camera and pipeline handler should remain as simple with possible, with high-level entry points. We can then implement helpers, in the form of classes deriving from PipelineHandler, or classes they implement helper objects (similar in concept to the V4L2BufferCache for instance) to avoid code duplication. It's a bit early to tell what form this will take, we probably need to look at two examples at least. > > > + data->cio2ToRequest_[buffer] = request; > > > + data->cio2_.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) > > > @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer) > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) > > > return; > > > > > > - cio2_.output_->queueBuffer(buffer); > > > + /* Remove association between CIO2 buffer an Request. */ > > > + cio2ToRequest_.erase(buffer); > > > + cio2_.putBuffer(buffer); > > > } > > > > > > /** > > > @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size, > > > int CIO2Device::allocateBuffers() > > > { > > > int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); > > > - if (ret < 0) > > > + if (ret < 0) { > > > LOG(IPU3, Error) << "Failed to allocate CIO2 buffers"; > > > > There's already a message printed by the V4L2VideoDevice class, do we > > need another one here ? > > I'm all for dropping it ;-) > > > > + return ret; > > > + } > > > + > > > + for (std::unique_ptr<FrameBuffer> &buffer : buffers_) > > > + availableBuffers_.push(buffer.get()); > > > > > > return ret; > > > } > > > > > > void CIO2Device::freeBuffers() > > > { > > > + while (!availableBuffers_.empty()) > > > + availableBuffers_.pop(); > > > + > > > > How about > > > > availableBuffers_ = {}; > > Nice idea. > > > ? > > > > > 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::queueBuffer(FrameBuffer *buffer) > > > +{ > > > + int ret = output_->queueBuffer(buffer); > > > + if (ret) > > > + LOG(IPU3, Error) << "Failed to queue CIO2 buffer"; > > > + > > > + return ret; > > > > Do we need this wrapper, can't we call > > cio2_.output_->queueBuffer(buffer) in the caller like we used to ? > > Not really, I added it as I think this driver is quiet messy and > breaking things apart could be a good thing. Will drop for v1 and then > we can refactor on top to make it more readable. > > > > +} > > > + > > > 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(); > > > } > > >
Hi Jacopo, On Wed, Mar 25, 2020 at 12:13:39PM +0100, Jacopo Mondi wrote: > On Mon, Mar 16, 2020 at 03:41:45AM +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 the 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> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++----- > > 1 file changed, 58 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 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/drm_fourcc.h> > > @@ -119,6 +120,10 @@ public: > > int allocateBuffers(); > > void freeBuffers(); > > > > + FrameBuffer *getBuffer(); > > + void putBuffer(FrameBuffer *buffer); > > + int queueBuffer(FrameBuffer *buffer); > > + > > int start(); > > int stop(); > > > > @@ -130,6 +135,7 @@ public: > > > > private: > > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > > + std::queue<FrameBuffer *> availableBuffers_; > > }; > > > > class IPU3Stream : public Stream > > @@ -157,6 +163,8 @@ public: > > void imguInputBufferReady(FrameBuffer *buffer); > > void cio2BufferReady(FrameBuffer *buffer); > > > > + std::map<FrameBuffer *, Request *> cio2ToRequest_; > > + > > CIO2Device cio2_; > > ImgUDevice *imgu_; > > > > @@ -717,11 +725,20 @@ 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; > > very minor nit: empty line > > I don't have much to add to Laurent's review, I'm wondering if we need > some kind of locking when accessing the queue though. Everything runs in a single thread, so there's no need to. > > + data->cio2ToRequest_[buffer] = request; > > + data->cio2_.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) > > @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer) > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) > > return; > > > > - cio2_.output_->queueBuffer(buffer); > > + /* Remove association between CIO2 buffer an Request. */ > > + cio2ToRequest_.erase(buffer); > > + cio2_.putBuffer(buffer); > > } > > > > /** > > @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size, > > int CIO2Device::allocateBuffers() > > { > > int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); > > - if (ret < 0) > > + 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() > > { > > + while (!availableBuffers_.empty()) > > + availableBuffers_.pop(); > > + > > 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(); > > OT: > Is it me or I'm rightfully puzzled by the fact std::queue::pop() does > not return an reference to the popped element? It would need to be a copy, not a reference, which would be expensive for queues that store large elements. > Anyway, looks good to me, will wait for v1 for a tag. > > > + > > + return buffer; > > +} > > + > > +void CIO2Device::putBuffer(FrameBuffer *buffer) > > +{ > > + availableBuffers_.push(buffer); > > +} > > + > > +int CIO2Device::queueBuffer(FrameBuffer *buffer) > > +{ > > + int ret = output_->queueBuffer(buffer); > > + if (ret) > > + LOG(IPU3, Error) << "Failed to queue CIO2 buffer"; > > + > > + return ret; > > +} > > + > > 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(); > > } > >
Hi Niklas, On Mon, Mar 16, 2020 at 03:41:45AM +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 the 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> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++----- > 1 file changed, 58 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 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/drm_fourcc.h> > @@ -119,6 +120,10 @@ public: > int allocateBuffers(); > void freeBuffers(); > > + FrameBuffer *getBuffer(); > + void putBuffer(FrameBuffer *buffer); > + int queueBuffer(FrameBuffer *buffer); > + > int start(); > int stop(); > > @@ -130,6 +135,7 @@ public: > > private: > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > + std::queue<FrameBuffer *> availableBuffers_; > }; > > class IPU3Stream : public Stream > @@ -157,6 +163,8 @@ public: > void imguInputBufferReady(FrameBuffer *buffer); > void cio2BufferReady(FrameBuffer *buffer); > > + std::map<FrameBuffer *, Request *> cio2ToRequest_; > + > CIO2Device cio2_; > ImgUDevice *imgu_; > > @@ -717,11 +725,20 @@ 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; very minor nit: empty line I don't have much to add to Laurent's review, I'm wondering if we need some kind of locking when accessing the queue though. > + data->cio2ToRequest_[buffer] = request; > + data->cio2_.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) > @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer) > if (buffer->metadata().status == FrameMetadata::FrameCancelled) > return; > > - cio2_.output_->queueBuffer(buffer); > + /* Remove association between CIO2 buffer an Request. */ > + cio2ToRequest_.erase(buffer); > + cio2_.putBuffer(buffer); > } > > /** > @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size, > int CIO2Device::allocateBuffers() > { > int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); > - if (ret < 0) > + 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() > { > + while (!availableBuffers_.empty()) > + availableBuffers_.pop(); > + > 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(); OT: Is it me or I'm rightfully puzzled by the fact std::queue::pop() does not return an reference to the popped element? Anyway, looks good to me, will wait for v1 for a tag. Thanks j > + > + return buffer; > +} > + > +void CIO2Device::putBuffer(FrameBuffer *buffer) > +{ > + availableBuffers_.push(buffer); > +} > + > +int CIO2Device::queueBuffer(FrameBuffer *buffer) > +{ > + int ret = output_->queueBuffer(buffer); > + if (ret) > + LOG(IPU3, Error) << "Failed to queue CIO2 buffer"; > + > + return ret; > +} > + > 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(); > } > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 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/drm_fourcc.h> @@ -119,6 +120,10 @@ public: int allocateBuffers(); void freeBuffers(); + FrameBuffer *getBuffer(); + void putBuffer(FrameBuffer *buffer); + int queueBuffer(FrameBuffer *buffer); + int start(); int stop(); @@ -130,6 +135,7 @@ public: private: std::vector<std::unique_ptr<FrameBuffer>> buffers_; + std::queue<FrameBuffer *> availableBuffers_; }; class IPU3Stream : public Stream @@ -157,6 +163,8 @@ public: void imguInputBufferReady(FrameBuffer *buffer); void cio2BufferReady(FrameBuffer *buffer); + std::map<FrameBuffer *, Request *> cio2ToRequest_; + CIO2Device cio2_; ImgUDevice *imgu_; @@ -717,11 +725,20 @@ 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; + data->cio2ToRequest_[buffer] = request; + data->cio2_.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) @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer) if (buffer->metadata().status == FrameMetadata::FrameCancelled) return; - cio2_.output_->queueBuffer(buffer); + /* Remove association between CIO2 buffer an Request. */ + cio2ToRequest_.erase(buffer); + cio2_.putBuffer(buffer); } /** @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size, int CIO2Device::allocateBuffers() { int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); - if (ret < 0) + 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() { + while (!availableBuffers_.empty()) + availableBuffers_.pop(); + 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::queueBuffer(FrameBuffer *buffer) +{ + int ret = output_->queueBuffer(buffer); + if (ret) + LOG(IPU3, Error) << "Failed to queue CIO2 buffer"; + + return ret; +} + 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 the 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> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++----- 1 file changed, 58 insertions(+), 11 deletions(-)