Message ID | 20200623020930.1781469-10-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, Thank you for the patch. On Tue, Jun 23, 2020 at 04:09:29AM +0200, Niklas Söderlund wrote: > The allocation and freeing of buffers for the CIO2 is handled in the > IPU3 pipeline handlers start() and stop() functions. These functions > also call CIO2Device start() and stop() at the appropriate times so > move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce > the complexity of the exposed interface. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > * Changes since v2 > - Stop IMGU before CIO2 > > * Changes since v1 > - Fix potential leaking of buffers if start fails. > --- > src/libcamera/pipeline/ipu3/cio2.cpp | 63 +++++++++++++--------------- > src/libcamera/pipeline/ipu3/cio2.h | 2 - > src/libcamera/pipeline/ipu3/ipu3.cpp | 14 ++----- > 3 files changed, 32 insertions(+), 47 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index 3d7348782b0fa6cb..efaa460b246697a6 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -207,44 +207,12 @@ CIO2Device::generateConfiguration(const Size &desiredSize) const > return cfg; > } > > -/** > - * \brief Allocate frame buffers for the CIO2 output > - * > - * Allocate frame buffers in the CIO2 video device to be used to capture frames > - * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_ > - * vector. > - * > - * \return Number of buffers allocated or negative error code > - */ > -int CIO2Device::allocateBuffers() > -{ > - int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); > - if (ret < 0) > - return ret; > - > - for (std::unique_ptr<FrameBuffer> &buffer : buffers_) > - availableBuffers_.push(buffer.get()); > - > - return ret; > -} > - > int CIO2Device::exportBuffers(unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > return output_->exportBuffers(count, buffers); > } > > -void CIO2Device::freeBuffers() > -{ > - /* The default std::queue constructor is explicit with gcc 5 and 6. */ > - availableBuffers_ = std::queue<FrameBuffer *>{}; > - > - buffers_.clear(); > - > - if (output_->releaseBuffers()) > - LOG(IPU3, Error) << "Failed to release CIO2 buffers"; > -} > - > FrameBuffer *CIO2Device::getBuffer() > { > if (availableBuffers_.empty()) { > @@ -266,12 +234,39 @@ void CIO2Device::putBuffer(FrameBuffer *buffer) > > int CIO2Device::start() > { > - return output_->streamOn(); > + int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); > + if (ret < 0) > + return ret; > + > + for (std::unique_ptr<FrameBuffer> &buffer : buffers_) > + availableBuffers_.push(buffer.get()); > + > + ret = output_->streamOn(); > + if (ret) { > + /* The default std::queue constructor is explicit with gcc 5 and 6. */ > + availableBuffers_ = std::queue<FrameBuffer *>{}; > + > + buffers_.clear(); > + > + output_->releaseBuffers(); > + } > + > + return ret; > } > > int CIO2Device::stop() > { > - return output_->streamOff(); > + int ret = output_->streamOff(); > + > + /* The default std::queue constructor is explicit with gcc 5 and 6. */ > + availableBuffers_ = std::queue<FrameBuffer *>{}; > + > + buffers_.clear(); > + > + if (output_->releaseBuffers()) > + LOG(IPU3, Error) << "Failed to release CIO2 buffers"; > + This duplicates code, I don't think it's nice :-( Is there an issue with keeping CIO2Device::freeBuffers() and making it private ? > + return ret; > } > > int CIO2Device::queueBuffer(FrameBuffer *buffer) > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > index cc898f13fd73f865..405e6c03755367c4 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.h > +++ b/src/libcamera/pipeline/ipu3/cio2.h > @@ -37,10 +37,8 @@ public: > > StreamConfiguration generateConfiguration(const Size &desiredSize) const; > > - int allocateBuffers(); > int exportBuffers(unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > - void freeBuffers(); > > FrameBuffer *getBuffer(); > void putBuffer(FrameBuffer *buffer); > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2d1ec707ea4b08fe..4818027e8db1f7a3 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -658,15 +658,10 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > int PipelineHandlerIPU3::allocateBuffers(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > - CIO2Device *cio2 = &data->cio2_; > ImgUDevice *imgu = data->imgu_; > unsigned int bufferCount; > int ret; > > - ret = cio2->allocateBuffers(); > - if (ret < 0) > - return ret; > - > bufferCount = std::max({ > data->outStream_.configuration().bufferCount, > data->vfStream_.configuration().bufferCount, > @@ -674,10 +669,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera) > }); > > ret = imgu->allocateBuffers(data, bufferCount); > - if (ret < 0) { > - cio2->freeBuffers(); > + if (ret < 0) > return ret; > - } > > return 0; > } > @@ -686,7 +679,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > > - data->cio2_.freeBuffers(); > data->imgu_->freeBuffers(data); > > return 0; > @@ -731,10 +723,10 @@ error: > void PipelineHandlerIPU3::stop(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > - int ret; > + int ret = 0; > > - ret = data->cio2_.stop(); > ret |= data->imgu_->stop(); > + ret |= data->cio2_.stop(); > if (ret) > LOG(IPU3, Warning) << "Failed to stop camera " > << camera->name();
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index 3d7348782b0fa6cb..efaa460b246697a6 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -207,44 +207,12 @@ CIO2Device::generateConfiguration(const Size &desiredSize) const return cfg; } -/** - * \brief Allocate frame buffers for the CIO2 output - * - * Allocate frame buffers in the CIO2 video device to be used to capture frames - * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_ - * vector. - * - * \return Number of buffers allocated or negative error code - */ -int CIO2Device::allocateBuffers() -{ - int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); - if (ret < 0) - return ret; - - for (std::unique_ptr<FrameBuffer> &buffer : buffers_) - availableBuffers_.push(buffer.get()); - - return ret; -} - int CIO2Device::exportBuffers(unsigned int count, std::vector<std::unique_ptr<FrameBuffer>> *buffers) { return output_->exportBuffers(count, buffers); } -void CIO2Device::freeBuffers() -{ - /* The default std::queue constructor is explicit with gcc 5 and 6. */ - availableBuffers_ = std::queue<FrameBuffer *>{}; - - buffers_.clear(); - - if (output_->releaseBuffers()) - LOG(IPU3, Error) << "Failed to release CIO2 buffers"; -} - FrameBuffer *CIO2Device::getBuffer() { if (availableBuffers_.empty()) { @@ -266,12 +234,39 @@ void CIO2Device::putBuffer(FrameBuffer *buffer) int CIO2Device::start() { - return output_->streamOn(); + int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_); + if (ret < 0) + return ret; + + for (std::unique_ptr<FrameBuffer> &buffer : buffers_) + availableBuffers_.push(buffer.get()); + + ret = output_->streamOn(); + if (ret) { + /* The default std::queue constructor is explicit with gcc 5 and 6. */ + availableBuffers_ = std::queue<FrameBuffer *>{}; + + buffers_.clear(); + + output_->releaseBuffers(); + } + + return ret; } int CIO2Device::stop() { - return output_->streamOff(); + int ret = output_->streamOff(); + + /* The default std::queue constructor is explicit with gcc 5 and 6. */ + availableBuffers_ = std::queue<FrameBuffer *>{}; + + buffers_.clear(); + + if (output_->releaseBuffers()) + LOG(IPU3, Error) << "Failed to release CIO2 buffers"; + + return ret; } int CIO2Device::queueBuffer(FrameBuffer *buffer) diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h index cc898f13fd73f865..405e6c03755367c4 100644 --- a/src/libcamera/pipeline/ipu3/cio2.h +++ b/src/libcamera/pipeline/ipu3/cio2.h @@ -37,10 +37,8 @@ public: StreamConfiguration generateConfiguration(const Size &desiredSize) const; - int allocateBuffers(); int exportBuffers(unsigned int count, std::vector<std::unique_ptr<FrameBuffer>> *buffers); - void freeBuffers(); FrameBuffer *getBuffer(); void putBuffer(FrameBuffer *buffer); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 2d1ec707ea4b08fe..4818027e8db1f7a3 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -658,15 +658,10 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, int PipelineHandlerIPU3::allocateBuffers(Camera *camera) { IPU3CameraData *data = cameraData(camera); - CIO2Device *cio2 = &data->cio2_; ImgUDevice *imgu = data->imgu_; unsigned int bufferCount; int ret; - ret = cio2->allocateBuffers(); - if (ret < 0) - return ret; - bufferCount = std::max({ data->outStream_.configuration().bufferCount, data->vfStream_.configuration().bufferCount, @@ -674,10 +669,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera) }); ret = imgu->allocateBuffers(data, bufferCount); - if (ret < 0) { - cio2->freeBuffers(); + if (ret < 0) return ret; - } return 0; } @@ -686,7 +679,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) { IPU3CameraData *data = cameraData(camera); - data->cio2_.freeBuffers(); data->imgu_->freeBuffers(data); return 0; @@ -731,10 +723,10 @@ error: void PipelineHandlerIPU3::stop(Camera *camera) { IPU3CameraData *data = cameraData(camera); - int ret; + int ret = 0; - ret = data->cio2_.stop(); ret |= data->imgu_->stop(); + ret |= data->cio2_.stop(); if (ret) LOG(IPU3, Warning) << "Failed to stop camera " << camera->name();