| Message ID | 20200918094233.5273-11-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | e1784ca88359827824f3b5a1efc60c15c9783564 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On 18/09/2020 10:42, Naushir Patuck wrote: > By using a map container, we can easily insert/remove buffers from the > buffer list. This will be required to track buffers allocated externally > and passed to the pipeline handler through a Request. > > Replace the buffer index tracking with an id generated internally by the > stream object. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------ > .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++----- > .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++-- > 3 files changed, 82 insertions(+), 35 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 7d91188c..51544233 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > int PipelineHandlerRPi::prepareBuffers(Camera *camera) > { > RPiCameraData *data = cameraData(camera); > - unsigned int index; > int ret; > > /* > @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > * This will allow us to identify buffers passed between the pipeline > * handler and the IPA. > */ > - index = 0; > for (auto const &b : data->isp_[Isp::Stats].getBuffers()) { > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index, > - .planes = b->planes() }); > - index++; > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first, > + .planes = b.second->planes() }); > } > > - index = 0; > for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) { > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index, > - .planes = b->planes() }); > - index++; > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first, > + .planes = b.second->planes() }); > } > > data->ipa_->mapBuffers(data->ipaBuffers_); > @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > return; > > for (RPi::RPiStream &s : unicam_) { > - index = s.getBufferIndex(buffer); > + index = s.getBufferId(buffer); > if (index != -1) { > stream = &s; > break; > @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer) > return; > > LOG(RPI, Debug) << "Stream ISP Input buffer complete" > - << ", buffer id " << unicam_[Unicam::Image].getBufferIndex(buffer) > + << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) > << ", timestamp: " << buffer->metadata().timestamp; > > /* The ISP input buffer gets re-queued into Unicam. */ > @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) > return; > > for (RPi::RPiStream &s : isp_) { > - index = s.getBufferIndex(buffer); > + index = s.getBufferId(buffer); > if (index != -1) { > stream = &s; > break; > @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline() > /* Set our state to say the pipeline is active. */ > state_ = State::Busy; > > - unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer); > - unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer); > + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer); > + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > LOG(RPI, Debug) << "Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:" > - << " Bayer buffer id: " << bayerIndex > - << " Embedded buffer id: " << embeddedIndex; > + << " Bayer buffer id: " << bayerId > + << " Embedded buffer id: " << embeddedId; > > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE; > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex, > - RPiIpaMask::BAYER_DATA | bayerIndex }; > + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId, > + RPiIpaMask::BAYER_DATA | bayerId }; > ipa_->processEvent(op); > } > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > index 80170d64..aee0aa2d 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const > > void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > - std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_), > - [](std::unique_ptr<FrameBuffer> &b) { return b.get(); }); > + for (auto const &buffer : *buffers) > + bufferMap_.emplace(id_.get(), buffer.get()); > } > > -const std::vector<FrameBuffer *> &RPiStream::getBuffers() const > +const BufferMap &RPiStream::getBuffers() const > { > - return bufferList_; > + return bufferMap_; > } > > -int RPiStream::getBufferIndex(FrameBuffer *buffer) const > +int RPiStream::getBufferId(FrameBuffer *buffer) const > { > if (importOnly_) > return -1; > > - /* Find the buffer in the list, and return the index position. */ > - auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer); > - if (it != bufferList_.end()) > - return std::distance(bufferList_.begin(), it); > + /* Find the buffer in the map, and return the buffer id. */ > + auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(), > + [&buffer](auto const &p) { return p.second == buffer; }); > > - return -1; > + if (it == bufferMap_.end()) > + return -1; > + > + return it->first; > } > > int RPiStream::prepareBuffers(unsigned int count) > @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count) > } > > /* We must import all internal/external exported buffers. */ > - count = bufferList_.size(); > + count = bufferMap_.size(); > } > > return dev_->importBuffers(count); > @@ -139,6 +141,9 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) > /* Push this buffer back into the queue to be used again. */ > availableBuffers_.push(buffer); > > + /* Allow the buffer id to be reused. */ > + id_.release(getBufferId(buffer)); > + > /* > * Do we have any Request buffers that are waiting to be queued? > * If so, do it now as availableBuffers_ will not be empty. > @@ -196,12 +201,13 @@ void RPiStream::clearBuffers() > availableBuffers_ = std::queue<FrameBuffer *>{}; > requestBuffers_ = std::queue<FrameBuffer *>{}; > internalBuffers_.clear(); > - bufferList_.clear(); > + bufferMap_.clear(); > + id_.reset(); > } > > int RPiStream::queueToDevice(FrameBuffer *buffer) > { > - LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferIndex(buffer) > + LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > << " for " << name_; > > int ret = dev_->queueBuffer(buffer); > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > index 959e9147..df986367 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > @@ -9,8 +9,10 @@ > > #include <queue> > #include <string> > +#include <unordered_map> > #include <vector> > > +#include <libcamera/ipa/raspberrypi.h> > #include <libcamera/stream.h> > > #include "libcamera/internal/v4l2_videodevice.h" > @@ -19,6 +21,8 @@ namespace libcamera { > > namespace RPi { > > +using BufferMap = std::unordered_map<unsigned int, FrameBuffer *>; > + > /* > * Device stream abstraction for either an internal or external stream. > * Used for both Unicam and the ISP. > @@ -27,12 +31,13 @@ class RPiStream : public Stream > { > public: > RPiStream() > + : id_(RPiIpaMask::ID) > { > } > > RPiStream(const char *name, MediaEntity *dev, bool importOnly = false) > : external_(false), importOnly_(importOnly), name_(name), > - dev_(std::make_unique<V4L2VideoDevice>(dev)) > + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiIpaMask::ID) > { > } > > @@ -45,8 +50,8 @@ public: > bool isExternal() const; > > void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers); > - const std::vector<FrameBuffer *> &getBuffers() const; > - int getBufferIndex(FrameBuffer *buffer) const; > + const BufferMap &getBuffers() const; > + int getBufferId(FrameBuffer *buffer) const; > > int prepareBuffers(unsigned int count); > int queueBuffer(FrameBuffer *buffer); > @@ -56,6 +61,44 @@ public: > void releaseBuffers(); > > private: > + class IdGenerator > + { > + public: > + IdGenerator(int max) Should this provide a default value for max? Perhaps int max = 32? (See below). > + : max_(max), id_(0) > + { > + } > + > + int get() > + { > + int id; > + if (!recycle_.empty()) { > + id = recycle_.front(); > + recycle_.pop(); > + } else { > + id = id_++; > + ASSERT(id_ <= max_); > + } > + return id; > + } > + > + void release(int id) > + { > + recycle_.push(id); > + } > + > + void reset() > + { > + id_ = 0; > + recycle_ = {}; > + } > + > + private: > + int max_; > + int id_; > + std::queue<int> recycle_; > + }; > + > void clearBuffers(); > int queueToDevice(FrameBuffer *buffer); > > @@ -74,8 +117,11 @@ private: > /* The actual device stream. */ > std::unique_ptr<V4L2VideoDevice> dev_; > > + /* Tracks a unique id key for the bufferMap_ */ > + IdGenerator id_; The only constructor for IdGenerator takes an int 'max' value, to assign to max_ which has the assertion check. That sounds useful to me for validation, but I can't see how this works (I'm probably missing something). With no default parameter, and nothing declared in the class definition as a default value, what is max_ set to ? (Some how I assume it would be zero?, but I would expect the compiler to complain too) Actually, I think what probably happens is it uses a 'default constructor' and leaves the id_ and max_ values uninitialised, so it's probably works by chance ? Other than that, I think this is neat. A quick and fast solution to the problem, and we never expect recycle queue to grow beyond the maximum quantity of unique buffers running in the system at any one time. With the max_ issue cleared: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> There shouldn't be any need to resend the whole series for this. If it does need fixing, either send an 8.1 of just this patch, or we can fix while applying perhaps, if it's just providing the default value. -- Kieran > + > /* All frame buffers associated with this device stream. */ > - std::vector<FrameBuffer *> bufferList_; > + BufferMap bufferMap_; > > /* > * List of frame buffers that we can use if none have been provided by >
Hi Kieran, Thank you for your review. On Fri, 18 Sep 2020 at 13:53, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Naush, > > On 18/09/2020 10:42, Naushir Patuck wrote: > > By using a map container, we can easily insert/remove buffers from the > > buffer list. This will be required to track buffers allocated externally > > and passed to the pipeline handler through a Request. > > > > Replace the buffer index tracking with an id generated internally by the > > stream object. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------ > > .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++----- > > .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++-- > > 3 files changed, 82 insertions(+), 35 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 7d91188c..51544233 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > > int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > { > > RPiCameraData *data = cameraData(camera); > > - unsigned int index; > > int ret; > > > > /* > > @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > * This will allow us to identify buffers passed between the pipeline > > * handler and the IPA. > > */ > > - index = 0; > > for (auto const &b : data->isp_[Isp::Stats].getBuffers()) { > > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index, > > - .planes = b->planes() }); > > - index++; > > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first, > > + .planes = b.second->planes() }); > > } > > > > - index = 0; > > for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) { > > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index, > > - .planes = b->planes() }); > > - index++; > > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first, > > + .planes = b.second->planes() }); > > } > > > > data->ipa_->mapBuffers(data->ipaBuffers_); > > @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > return; > > > > for (RPi::RPiStream &s : unicam_) { > > - index = s.getBufferIndex(buffer); > > + index = s.getBufferId(buffer); > > if (index != -1) { > > stream = &s; > > break; > > @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer) > > return; > > > > LOG(RPI, Debug) << "Stream ISP Input buffer complete" > > - << ", buffer id " << unicam_[Unicam::Image].getBufferIndex(buffer) > > + << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) > > << ", timestamp: " << buffer->metadata().timestamp; > > > > /* The ISP input buffer gets re-queued into Unicam. */ > > @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) > > return; > > > > for (RPi::RPiStream &s : isp_) { > > - index = s.getBufferIndex(buffer); > > + index = s.getBufferId(buffer); > > if (index != -1) { > > stream = &s; > > break; > > @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline() > > /* Set our state to say the pipeline is active. */ > > state_ = State::Busy; > > > > - unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer); > > - unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer); > > + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer); > > + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > > > LOG(RPI, Debug) << "Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:" > > - << " Bayer buffer id: " << bayerIndex > > - << " Embedded buffer id: " << embeddedIndex; > > + << " Bayer buffer id: " << bayerId > > + << " Embedded buffer id: " << embeddedId; > > > > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE; > > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex, > > - RPiIpaMask::BAYER_DATA | bayerIndex }; > > + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId, > > + RPiIpaMask::BAYER_DATA | bayerId }; > > ipa_->processEvent(op); > > } > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > index 80170d64..aee0aa2d 100644 > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const > > > > void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > { > > - std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_), > > - [](std::unique_ptr<FrameBuffer> &b) { return b.get(); }); > > + for (auto const &buffer : *buffers) > > + bufferMap_.emplace(id_.get(), buffer.get()); > > } > > > > -const std::vector<FrameBuffer *> &RPiStream::getBuffers() const > > +const BufferMap &RPiStream::getBuffers() const > > { > > - return bufferList_; > > + return bufferMap_; > > } > > > > -int RPiStream::getBufferIndex(FrameBuffer *buffer) const > > +int RPiStream::getBufferId(FrameBuffer *buffer) const > > { > > if (importOnly_) > > return -1; > > > > - /* Find the buffer in the list, and return the index position. */ > > - auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer); > > - if (it != bufferList_.end()) > > - return std::distance(bufferList_.begin(), it); > > + /* Find the buffer in the map, and return the buffer id. */ > > + auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(), > > + [&buffer](auto const &p) { return p.second == buffer; }); > > > > - return -1; > > + if (it == bufferMap_.end()) > > + return -1; > > + > > + return it->first; > > } > > > > int RPiStream::prepareBuffers(unsigned int count) > > @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count) > > } > > > > /* We must import all internal/external exported buffers. */ > > - count = bufferList_.size(); > > + count = bufferMap_.size(); > > } > > > > return dev_->importBuffers(count); > > @@ -139,6 +141,9 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) > > /* Push this buffer back into the queue to be used again. */ > > availableBuffers_.push(buffer); > > > > + /* Allow the buffer id to be reused. */ > > + id_.release(getBufferId(buffer)); > > + > > /* > > * Do we have any Request buffers that are waiting to be queued? > > * If so, do it now as availableBuffers_ will not be empty. > > @@ -196,12 +201,13 @@ void RPiStream::clearBuffers() > > availableBuffers_ = std::queue<FrameBuffer *>{}; > > requestBuffers_ = std::queue<FrameBuffer *>{}; > > internalBuffers_.clear(); > > - bufferList_.clear(); > > + bufferMap_.clear(); > > + id_.reset(); > > } > > > > int RPiStream::queueToDevice(FrameBuffer *buffer) > > { > > - LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferIndex(buffer) > > + LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > > << " for " << name_; > > > > int ret = dev_->queueBuffer(buffer); > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > index 959e9147..df986367 100644 > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > @@ -9,8 +9,10 @@ > > > > #include <queue> > > #include <string> > > +#include <unordered_map> > > #include <vector> > > > > +#include <libcamera/ipa/raspberrypi.h> > > #include <libcamera/stream.h> > > > > #include "libcamera/internal/v4l2_videodevice.h" > > @@ -19,6 +21,8 @@ namespace libcamera { > > > > namespace RPi { > > > > +using BufferMap = std::unordered_map<unsigned int, FrameBuffer *>; > > + > > /* > > * Device stream abstraction for either an internal or external stream. > > * Used for both Unicam and the ISP. > > @@ -27,12 +31,13 @@ class RPiStream : public Stream > > { > > public: > > RPiStream() > > + : id_(RPiIpaMask::ID) > > { > > } > > > > RPiStream(const char *name, MediaEntity *dev, bool importOnly = false) > > : external_(false), importOnly_(importOnly), name_(name), > > - dev_(std::make_unique<V4L2VideoDevice>(dev)) > > + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiIpaMask::ID) > > { > > } > > > > @@ -45,8 +50,8 @@ public: > > bool isExternal() const; > > > > void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > - const std::vector<FrameBuffer *> &getBuffers() const; > > - int getBufferIndex(FrameBuffer *buffer) const; > > + const BufferMap &getBuffers() const; > > + int getBufferId(FrameBuffer *buffer) const; > > > > int prepareBuffers(unsigned int count); > > int queueBuffer(FrameBuffer *buffer); > > @@ -56,6 +61,44 @@ public: > > void releaseBuffers(); > > > > private: > > + class IdGenerator > > + { > > + public: > > + IdGenerator(int max) > > Should this provide a default value for max? > > Perhaps int max = 32? > > (See below). > > > + : max_(max), id_(0) > > + { > > + } > > + > > + int get() > > + { > > + int id; > > + if (!recycle_.empty()) { > > + id = recycle_.front(); > > + recycle_.pop(); > > + } else { > > + id = id_++; > > + ASSERT(id_ <= max_); > > + } > > + return id; > > + } > > + > > + void release(int id) > > + { > > + recycle_.push(id); > > + } > > + > > + void reset() > > + { > > + id_ = 0; > > + recycle_ = {}; > > + } > > + > > + private: > > + int max_; > > + int id_; > > + std::queue<int> recycle_; > > + }; > > + > > void clearBuffers(); > > int queueToDevice(FrameBuffer *buffer); > > > > @@ -74,8 +117,11 @@ private: > > /* The actual device stream. */ > > std::unique_ptr<V4L2VideoDevice> dev_; > > > > + /* Tracks a unique id key for the bufferMap_ */ > > + IdGenerator id_; > > The only constructor for IdGenerator takes an int 'max' value, to assign > to max_ which has the assertion check. > > That sounds useful to me for validation, but I can't see how this works > (I'm probably missing something). > > With no default parameter, and nothing declared in the class definition > as a default value, what is max_ set to ? > (Some how I assume it would be zero?, but I would expect the compiler to > complain too) Unless I misunderstood your comment above, the IdGenerator class is initialised in the RPiStream contstructors: RPiStream() : id_(RPiBufferMask::ID) { } RPiStream(const char *name, MediaEntity *dev, bool importOnly = false) : external_(false), importOnly_(importOnly), name_(name), dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiBufferMask::ID) { } IdGenerator::max_ takes the value RPiBufferMask::ID (0xffff) in this case. Given no default value is used, the compiler ought to shout loudly if I were to construct an IdGenerator instance without a max parameter. Does that answer your query? Regards, Naush > > Actually, I think what probably happens is it uses a 'default > constructor' and leaves the id_ and max_ values uninitialised, so it's > probably works by chance ? > > Other than that, I think this is neat. A quick and fast solution to the > problem, and we never expect recycle queue to grow beyond the maximum > quantity of unique buffers running in the system at any one time. > > With the max_ issue cleared: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > There shouldn't be any need to resend the whole series for this. > If it does need fixing, either send an 8.1 of just this patch, or we can > fix while applying perhaps, if it's just providing the default value. > -- > Kieran > > > > > > > + > > /* All frame buffers associated with this device stream. */ > > - std::vector<FrameBuffer *> bufferList_; > > + BufferMap bufferMap_; > > > > /* > > * List of frame buffers that we can use if none have been provided by > > > > -- > Regards > -- > Kieran
Hi Naush, On 18/09/2020 14:00, Naushir Patuck wrote: > Hi Kieran, > > Thank you for your review. > > On Fri, 18 Sep 2020 at 13:53, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> >> Hi Naush, >> >> On 18/09/2020 10:42, Naushir Patuck wrote: >>> By using a map container, we can easily insert/remove buffers from the >>> buffer list. This will be required to track buffers allocated externally >>> and passed to the pipeline handler through a Request. >>> >>> Replace the buffer index tracking with an id generated internally by the >>> stream object. >>> >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >>> --- >>> .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------ >>> .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++----- >>> .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++-- >>> 3 files changed, 82 insertions(+), 35 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> index 7d91188c..51544233 100644 >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) >>> int PipelineHandlerRPi::prepareBuffers(Camera *camera) >>> { >>> RPiCameraData *data = cameraData(camera); >>> - unsigned int index; >>> int ret; >>> >>> /* >>> @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) >>> * This will allow us to identify buffers passed between the pipeline >>> * handler and the IPA. >>> */ >>> - index = 0; >>> for (auto const &b : data->isp_[Isp::Stats].getBuffers()) { >>> - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index, >>> - .planes = b->planes() }); >>> - index++; >>> + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first, >>> + .planes = b.second->planes() }); >>> } >>> >>> - index = 0; >>> for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) { >>> - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index, >>> - .planes = b->planes() }); >>> - index++; >>> + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first, >>> + .planes = b.second->planes() }); >>> } >>> >>> data->ipa_->mapBuffers(data->ipaBuffers_); >>> @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) >>> return; >>> >>> for (RPi::RPiStream &s : unicam_) { >>> - index = s.getBufferIndex(buffer); >>> + index = s.getBufferId(buffer); >>> if (index != -1) { >>> stream = &s; >>> break; >>> @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer) >>> return; >>> >>> LOG(RPI, Debug) << "Stream ISP Input buffer complete" >>> - << ", buffer id " << unicam_[Unicam::Image].getBufferIndex(buffer) >>> + << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) >>> << ", timestamp: " << buffer->metadata().timestamp; >>> >>> /* The ISP input buffer gets re-queued into Unicam. */ >>> @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) >>> return; >>> >>> for (RPi::RPiStream &s : isp_) { >>> - index = s.getBufferIndex(buffer); >>> + index = s.getBufferId(buffer); >>> if (index != -1) { >>> stream = &s; >>> break; >>> @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline() >>> /* Set our state to say the pipeline is active. */ >>> state_ = State::Busy; >>> >>> - unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer); >>> - unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer); >>> + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer); >>> + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); >>> >>> LOG(RPI, Debug) << "Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:" >>> - << " Bayer buffer id: " << bayerIndex >>> - << " Embedded buffer id: " << embeddedIndex; >>> + << " Bayer buffer id: " << bayerId >>> + << " Embedded buffer id: " << embeddedId; >>> >>> op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE; >>> - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex, >>> - RPiIpaMask::BAYER_DATA | bayerIndex }; >>> + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId, >>> + RPiIpaMask::BAYER_DATA | bayerId }; >>> ipa_->processEvent(op); >>> } >>> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp >>> index 80170d64..aee0aa2d 100644 >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp >>> @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const >>> >>> void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers) >>> { >>> - std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_), >>> - [](std::unique_ptr<FrameBuffer> &b) { return b.get(); }); >>> + for (auto const &buffer : *buffers) >>> + bufferMap_.emplace(id_.get(), buffer.get()); >>> } >>> >>> -const std::vector<FrameBuffer *> &RPiStream::getBuffers() const >>> +const BufferMap &RPiStream::getBuffers() const >>> { >>> - return bufferList_; >>> + return bufferMap_; >>> } >>> >>> -int RPiStream::getBufferIndex(FrameBuffer *buffer) const >>> +int RPiStream::getBufferId(FrameBuffer *buffer) const >>> { >>> if (importOnly_) >>> return -1; >>> >>> - /* Find the buffer in the list, and return the index position. */ >>> - auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer); >>> - if (it != bufferList_.end()) >>> - return std::distance(bufferList_.begin(), it); >>> + /* Find the buffer in the map, and return the buffer id. */ >>> + auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(), >>> + [&buffer](auto const &p) { return p.second == buffer; }); >>> >>> - return -1; >>> + if (it == bufferMap_.end()) >>> + return -1; >>> + >>> + return it->first; >>> } >>> >>> int RPiStream::prepareBuffers(unsigned int count) >>> @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count) >>> } >>> >>> /* We must import all internal/external exported buffers. */ >>> - count = bufferList_.size(); >>> + count = bufferMap_.size(); >>> } >>> >>> return dev_->importBuffers(count); >>> @@ -139,6 +141,9 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) >>> /* Push this buffer back into the queue to be used again. */ >>> availableBuffers_.push(buffer); >>> >>> + /* Allow the buffer id to be reused. */ >>> + id_.release(getBufferId(buffer)); >>> + >>> /* >>> * Do we have any Request buffers that are waiting to be queued? >>> * If so, do it now as availableBuffers_ will not be empty. >>> @@ -196,12 +201,13 @@ void RPiStream::clearBuffers() >>> availableBuffers_ = std::queue<FrameBuffer *>{}; >>> requestBuffers_ = std::queue<FrameBuffer *>{}; >>> internalBuffers_.clear(); >>> - bufferList_.clear(); >>> + bufferMap_.clear(); >>> + id_.reset(); >>> } >>> >>> int RPiStream::queueToDevice(FrameBuffer *buffer) >>> { >>> - LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferIndex(buffer) >>> + LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) >>> << " for " << name_; >>> >>> int ret = dev_->queueBuffer(buffer); >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h >>> index 959e9147..df986367 100644 >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h >>> @@ -9,8 +9,10 @@ >>> >>> #include <queue> >>> #include <string> >>> +#include <unordered_map> >>> #include <vector> >>> >>> +#include <libcamera/ipa/raspberrypi.h> >>> #include <libcamera/stream.h> >>> >>> #include "libcamera/internal/v4l2_videodevice.h" >>> @@ -19,6 +21,8 @@ namespace libcamera { >>> >>> namespace RPi { >>> >>> +using BufferMap = std::unordered_map<unsigned int, FrameBuffer *>; >>> + >>> /* >>> * Device stream abstraction for either an internal or external stream. >>> * Used for both Unicam and the ISP. >>> @@ -27,12 +31,13 @@ class RPiStream : public Stream >>> { >>> public: >>> RPiStream() >>> + : id_(RPiIpaMask::ID) >>> { >>> } >>> >>> RPiStream(const char *name, MediaEntity *dev, bool importOnly = false) >>> : external_(false), importOnly_(importOnly), name_(name), >>> - dev_(std::make_unique<V4L2VideoDevice>(dev)) >>> + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiIpaMask::ID) >>> { >>> } >>> >>> @@ -45,8 +50,8 @@ public: >>> bool isExternal() const; >>> >>> void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers); >>> - const std::vector<FrameBuffer *> &getBuffers() const; >>> - int getBufferIndex(FrameBuffer *buffer) const; >>> + const BufferMap &getBuffers() const; >>> + int getBufferId(FrameBuffer *buffer) const; >>> >>> int prepareBuffers(unsigned int count); >>> int queueBuffer(FrameBuffer *buffer); >>> @@ -56,6 +61,44 @@ public: >>> void releaseBuffers(); >>> >>> private: >>> + class IdGenerator >>> + { >>> + public: >>> + IdGenerator(int max) >> >> Should this provide a default value for max? >> >> Perhaps int max = 32? >> >> (See below). >> >>> + : max_(max), id_(0) >>> + { >>> + } >>> + >>> + int get() >>> + { >>> + int id; >>> + if (!recycle_.empty()) { >>> + id = recycle_.front(); >>> + recycle_.pop(); >>> + } else { >>> + id = id_++; >>> + ASSERT(id_ <= max_); >>> + } >>> + return id; >>> + } >>> + >>> + void release(int id) >>> + { >>> + recycle_.push(id); >>> + } >>> + >>> + void reset() >>> + { >>> + id_ = 0; >>> + recycle_ = {}; >>> + } >>> + >>> + private: >>> + int max_; >>> + int id_; >>> + std::queue<int> recycle_; >>> + }; >>> + >>> void clearBuffers(); >>> int queueToDevice(FrameBuffer *buffer); >>> >>> @@ -74,8 +117,11 @@ private: >>> /* The actual device stream. */ >>> std::unique_ptr<V4L2VideoDevice> dev_; >>> >>> + /* Tracks a unique id key for the bufferMap_ */ >>> + IdGenerator id_; >> >> The only constructor for IdGenerator takes an int 'max' value, to assign >> to max_ which has the assertion check. >> >> That sounds useful to me for validation, but I can't see how this works >> (I'm probably missing something). >> >> With no default parameter, and nothing declared in the class definition >> as a default value, what is max_ set to ? >> (Some how I assume it would be zero?, but I would expect the compiler to >> complain too) > > Unless I misunderstood your comment above, the IdGenerator class is > initialised in the RPiStream contstructors: Aha- that's what I'd missed, it *is* initialised ;-) I'm sorry for the noise. > > RPiStream() > : id_(RPiBufferMask::ID) > { > } > > RPiStream(const char *name, MediaEntity *dev, bool importOnly = false) > : external_(false), importOnly_(importOnly), name_(name), > dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiBufferMask::ID) > { > } > > IdGenerator::max_ takes the value RPiBufferMask::ID (0xffff) in this > case. Given no default value is used, the compiler ought to shout > loudly if I were to construct an IdGenerator instance without a max > parameter. Does that answer your query? Yup - I had completely missed that the id_ was being intialised by the stream, and somehow had it in my head that it was using a default initialiser. Thanks, This still stands, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> So I'll let Niklas take this on from here! -- Kieran > Regards, > Naush > > > >> >> Actually, I think what probably happens is it uses a 'default >> constructor' and leaves the id_ and max_ values uninitialised, so it's >> probably works by chance ? >> >> Other than that, I think this is neat. A quick and fast solution to the >> problem, and we never expect recycle queue to grow beyond the maximum >> quantity of unique buffers running in the system at any one time. >> >> With the max_ issue cleared: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> There shouldn't be any need to resend the whole series for this. >> If it does need fixing, either send an 8.1 of just this patch, or we can >> fix while applying perhaps, if it's just providing the default value. >> -- >> Kieran >> >> >> >> >> >>> + >>> /* All frame buffers associated with this device stream. */ >>> - std::vector<FrameBuffer *> bufferList_; >>> + BufferMap bufferMap_; >>> >>> /* >>> * List of frame buffers that we can use if none have been provided by >>> >> >> -- >> Regards >> -- >> Kieran
Hi Naushir, Thanks for your work. On 2020-09-18 10:42:31 +0100, Naushir Patuck wrote: > By using a map container, we can easily insert/remove buffers from the > buffer list. This will be required to track buffers allocated externally > and passed to the pipeline handler through a Request. > > Replace the buffer index tracking with an id generated internally by the > stream object. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------ > .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++----- > .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++-- > 3 files changed, 82 insertions(+), 35 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 7d91188c..51544233 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > int PipelineHandlerRPi::prepareBuffers(Camera *camera) > { > RPiCameraData *data = cameraData(camera); > - unsigned int index; > int ret; > > /* > @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > * This will allow us to identify buffers passed between the pipeline > * handler and the IPA. > */ > - index = 0; > for (auto const &b : data->isp_[Isp::Stats].getBuffers()) { > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index, > - .planes = b->planes() }); > - index++; > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first, > + .planes = b.second->planes() }); > } > > - index = 0; > for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) { > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index, > - .planes = b->planes() }); > - index++; > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first, > + .planes = b.second->planes() }); > } > > data->ipa_->mapBuffers(data->ipaBuffers_); > @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > return; > > for (RPi::RPiStream &s : unicam_) { > - index = s.getBufferIndex(buffer); > + index = s.getBufferId(buffer); > if (index != -1) { > stream = &s; > break; > @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer) > return; > > LOG(RPI, Debug) << "Stream ISP Input buffer complete" > - << ", buffer id " << unicam_[Unicam::Image].getBufferIndex(buffer) > + << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) > << ", timestamp: " << buffer->metadata().timestamp; > > /* The ISP input buffer gets re-queued into Unicam. */ > @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) > return; > > for (RPi::RPiStream &s : isp_) { > - index = s.getBufferIndex(buffer); > + index = s.getBufferId(buffer); > if (index != -1) { > stream = &s; > break; > @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline() > /* Set our state to say the pipeline is active. */ > state_ = State::Busy; > > - unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer); > - unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer); > + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer); > + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > LOG(RPI, Debug) << "Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:" > - << " Bayer buffer id: " << bayerIndex > - << " Embedded buffer id: " << embeddedIndex; > + << " Bayer buffer id: " << bayerId > + << " Embedded buffer id: " << embeddedId; > > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE; > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex, > - RPiIpaMask::BAYER_DATA | bayerIndex }; > + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId, > + RPiIpaMask::BAYER_DATA | bayerId }; > ipa_->processEvent(op); > } > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > index 80170d64..aee0aa2d 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const > > void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > - std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_), > - [](std::unique_ptr<FrameBuffer> &b) { return b.get(); }); > + for (auto const &buffer : *buffers) > + bufferMap_.emplace(id_.get(), buffer.get()); > } > > -const std::vector<FrameBuffer *> &RPiStream::getBuffers() const > +const BufferMap &RPiStream::getBuffers() const > { > - return bufferList_; > + return bufferMap_; > } > > -int RPiStream::getBufferIndex(FrameBuffer *buffer) const > +int RPiStream::getBufferId(FrameBuffer *buffer) const > { > if (importOnly_) > return -1; > > - /* Find the buffer in the list, and return the index position. */ > - auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer); > - if (it != bufferList_.end()) > - return std::distance(bufferList_.begin(), it); > + /* Find the buffer in the map, and return the buffer id. */ > + auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(), > + [&buffer](auto const &p) { return p.second == buffer; }); > > - return -1; > + if (it == bufferMap_.end()) > + return -1; > + > + return it->first; > } > > int RPiStream::prepareBuffers(unsigned int count) > @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count) > } > > /* We must import all internal/external exported buffers. */ > - count = bufferList_.size(); > + count = bufferMap_.size(); > } > > return dev_->importBuffers(count); > @@ -139,6 +141,9 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) > /* Push this buffer back into the queue to be used again. */ > availableBuffers_.push(buffer); > > + /* Allow the buffer id to be reused. */ > + id_.release(getBufferId(buffer)); > + > /* > * Do we have any Request buffers that are waiting to be queued? > * If so, do it now as availableBuffers_ will not be empty. > @@ -196,12 +201,13 @@ void RPiStream::clearBuffers() > availableBuffers_ = std::queue<FrameBuffer *>{}; > requestBuffers_ = std::queue<FrameBuffer *>{}; > internalBuffers_.clear(); > - bufferList_.clear(); > + bufferMap_.clear(); > + id_.reset(); > } > > int RPiStream::queueToDevice(FrameBuffer *buffer) > { > - LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferIndex(buffer) > + LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > << " for " << name_; > > int ret = dev_->queueBuffer(buffer); > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > index 959e9147..df986367 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > @@ -9,8 +9,10 @@ > > #include <queue> > #include <string> > +#include <unordered_map> > #include <vector> > > +#include <libcamera/ipa/raspberrypi.h> > #include <libcamera/stream.h> > > #include "libcamera/internal/v4l2_videodevice.h" > @@ -19,6 +21,8 @@ namespace libcamera { > > namespace RPi { > > +using BufferMap = std::unordered_map<unsigned int, FrameBuffer *>; > + > /* > * Device stream abstraction for either an internal or external stream. > * Used for both Unicam and the ISP. > @@ -27,12 +31,13 @@ class RPiStream : public Stream > { > public: > RPiStream() > + : id_(RPiIpaMask::ID) > { > } > > RPiStream(const char *name, MediaEntity *dev, bool importOnly = false) > : external_(false), importOnly_(importOnly), name_(name), > - dev_(std::make_unique<V4L2VideoDevice>(dev)) > + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiIpaMask::ID) > { > } > > @@ -45,8 +50,8 @@ public: > bool isExternal() const; > > void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers); > - const std::vector<FrameBuffer *> &getBuffers() const; > - int getBufferIndex(FrameBuffer *buffer) const; > + const BufferMap &getBuffers() const; > + int getBufferId(FrameBuffer *buffer) const; > > int prepareBuffers(unsigned int count); > int queueBuffer(FrameBuffer *buffer); > @@ -56,6 +61,44 @@ public: > void releaseBuffers(); > > private: > + class IdGenerator > + { > + public: > + IdGenerator(int max) > + : max_(max), id_(0) > + { > + } > + > + int get() > + { > + int id; > + if (!recycle_.empty()) { > + id = recycle_.front(); > + recycle_.pop(); > + } else { > + id = id_++; > + ASSERT(id_ <= max_); > + } > + return id; > + } > + > + void release(int id) > + { > + recycle_.push(id); > + } > + > + void reset() > + { > + id_ = 0; > + recycle_ = {}; > + } > + > + private: > + int max_; > + int id_; > + std::queue<int> recycle_; > + }; > + > void clearBuffers(); > int queueToDevice(FrameBuffer *buffer); > > @@ -74,8 +117,11 @@ private: > /* The actual device stream. */ > std::unique_ptr<V4L2VideoDevice> dev_; > > + /* Tracks a unique id key for the bufferMap_ */ > + IdGenerator id_; > + > /* All frame buffers associated with this device stream. */ > - std::vector<FrameBuffer *> bufferList_; > + BufferMap bufferMap_; > > /* > * List of frame buffers that we can use if none have been provided by > -- > 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/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 7d91188c..51544233 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) int PipelineHandlerRPi::prepareBuffers(Camera *camera) { RPiCameraData *data = cameraData(camera); - unsigned int index; int ret; /* @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) * This will allow us to identify buffers passed between the pipeline * handler and the IPA. */ - index = 0; for (auto const &b : data->isp_[Isp::Stats].getBuffers()) { - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index, - .planes = b->planes() }); - index++; + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first, + .planes = b.second->planes() }); } - index = 0; for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) { - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index, - .planes = b->planes() }); - index++; + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first, + .planes = b.second->planes() }); } data->ipa_->mapBuffers(data->ipaBuffers_); @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) return; for (RPi::RPiStream &s : unicam_) { - index = s.getBufferIndex(buffer); + index = s.getBufferId(buffer); if (index != -1) { stream = &s; break; @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer) return; LOG(RPI, Debug) << "Stream ISP Input buffer complete" - << ", buffer id " << unicam_[Unicam::Image].getBufferIndex(buffer) + << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) << ", timestamp: " << buffer->metadata().timestamp; /* The ISP input buffer gets re-queued into Unicam. */ @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) return; for (RPi::RPiStream &s : isp_) { - index = s.getBufferIndex(buffer); + index = s.getBufferId(buffer); if (index != -1) { stream = &s; break; @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline() /* Set our state to say the pipeline is active. */ state_ = State::Busy; - unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer); - unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer); + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer); + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); LOG(RPI, Debug) << "Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:" - << " Bayer buffer id: " << bayerIndex - << " Embedded buffer id: " << embeddedIndex; + << " Bayer buffer id: " << bayerId + << " Embedded buffer id: " << embeddedId; op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE; - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex, - RPiIpaMask::BAYER_DATA | bayerIndex }; + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId, + RPiIpaMask::BAYER_DATA | bayerId }; ipa_->processEvent(op); } diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp index 80170d64..aee0aa2d 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers) { - std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_), - [](std::unique_ptr<FrameBuffer> &b) { return b.get(); }); + for (auto const &buffer : *buffers) + bufferMap_.emplace(id_.get(), buffer.get()); } -const std::vector<FrameBuffer *> &RPiStream::getBuffers() const +const BufferMap &RPiStream::getBuffers() const { - return bufferList_; + return bufferMap_; } -int RPiStream::getBufferIndex(FrameBuffer *buffer) const +int RPiStream::getBufferId(FrameBuffer *buffer) const { if (importOnly_) return -1; - /* Find the buffer in the list, and return the index position. */ - auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer); - if (it != bufferList_.end()) - return std::distance(bufferList_.begin(), it); + /* Find the buffer in the map, and return the buffer id. */ + auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(), + [&buffer](auto const &p) { return p.second == buffer; }); - return -1; + if (it == bufferMap_.end()) + return -1; + + return it->first; } int RPiStream::prepareBuffers(unsigned int count) @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count) } /* We must import all internal/external exported buffers. */ - count = bufferList_.size(); + count = bufferMap_.size(); } return dev_->importBuffers(count); @@ -139,6 +141,9 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) /* Push this buffer back into the queue to be used again. */ availableBuffers_.push(buffer); + /* Allow the buffer id to be reused. */ + id_.release(getBufferId(buffer)); + /* * Do we have any Request buffers that are waiting to be queued? * If so, do it now as availableBuffers_ will not be empty. @@ -196,12 +201,13 @@ void RPiStream::clearBuffers() availableBuffers_ = std::queue<FrameBuffer *>{}; requestBuffers_ = std::queue<FrameBuffer *>{}; internalBuffers_.clear(); - bufferList_.clear(); + bufferMap_.clear(); + id_.reset(); } int RPiStream::queueToDevice(FrameBuffer *buffer) { - LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferIndex(buffer) + LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) << " for " << name_; int ret = dev_->queueBuffer(buffer); diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h index 959e9147..df986367 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h @@ -9,8 +9,10 @@ #include <queue> #include <string> +#include <unordered_map> #include <vector> +#include <libcamera/ipa/raspberrypi.h> #include <libcamera/stream.h> #include "libcamera/internal/v4l2_videodevice.h" @@ -19,6 +21,8 @@ namespace libcamera { namespace RPi { +using BufferMap = std::unordered_map<unsigned int, FrameBuffer *>; + /* * Device stream abstraction for either an internal or external stream. * Used for both Unicam and the ISP. @@ -27,12 +31,13 @@ class RPiStream : public Stream { public: RPiStream() + : id_(RPiIpaMask::ID) { } RPiStream(const char *name, MediaEntity *dev, bool importOnly = false) : external_(false), importOnly_(importOnly), name_(name), - dev_(std::make_unique<V4L2VideoDevice>(dev)) + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiIpaMask::ID) { } @@ -45,8 +50,8 @@ public: bool isExternal() const; void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers); - const std::vector<FrameBuffer *> &getBuffers() const; - int getBufferIndex(FrameBuffer *buffer) const; + const BufferMap &getBuffers() const; + int getBufferId(FrameBuffer *buffer) const; int prepareBuffers(unsigned int count); int queueBuffer(FrameBuffer *buffer); @@ -56,6 +61,44 @@ public: void releaseBuffers(); private: + class IdGenerator + { + public: + IdGenerator(int max) + : max_(max), id_(0) + { + } + + int get() + { + int id; + if (!recycle_.empty()) { + id = recycle_.front(); + recycle_.pop(); + } else { + id = id_++; + ASSERT(id_ <= max_); + } + return id; + } + + void release(int id) + { + recycle_.push(id); + } + + void reset() + { + id_ = 0; + recycle_ = {}; + } + + private: + int max_; + int id_; + std::queue<int> recycle_; + }; + void clearBuffers(); int queueToDevice(FrameBuffer *buffer); @@ -74,8 +117,11 @@ private: /* The actual device stream. */ std::unique_ptr<V4L2VideoDevice> dev_; + /* Tracks a unique id key for the bufferMap_ */ + IdGenerator id_; + /* All frame buffers associated with this device stream. */ - std::vector<FrameBuffer *> bufferList_; + BufferMap bufferMap_; /* * List of frame buffers that we can use if none have been provided by
By using a map container, we can easily insert/remove buffers from the buffer list. This will be required to track buffers allocated externally and passed to the pipeline handler through a Request. Replace the buffer index tracking with an id generated internally by the stream object. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------ .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++----- .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++-- 3 files changed, 82 insertions(+), 35 deletions(-)