[libcamera-devel] [PATCH v4 9/9] libcamera: pipeline: ipa: raspberrypi: Remove use of FrameBuffer cookie
Naushir Patuck
naush at raspberrypi.com
Tue Jul 21 14:38:00 CEST 2020
HI Kieran,
On Tue, 21 Jul 2020 at 12:28, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On 20/07/2020 10:13, Naushir Patuck wrote:
> > The FrameBuffer cookie may be set by the application, so this cannot
> > be set by the pipeline handler as well. Revert to using a simple index
> > into the buffer list to identify buffers passing to and from the IPA.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> I wonder if this means we should add an internal cookie or ability to
> identify the IPA buffers in the buffer class itself.
>
> But still, this looks like it works and fixes the issue.
I did think cookies were for internal use only :-) However, given the
simplicity of using a buffer index, an internal-only cookie probably
not needed.
Regards,
Naush
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > ---
> > .../pipeline/raspberrypi/raspberrypi.cpp | 60 ++++++++++---------
> > .../pipeline/raspberrypi/rpi_stream.cpp | 14 +++--
> > .../pipeline/raspberrypi/rpi_stream.h | 2 +-
> > 3 files changed, 40 insertions(+), 36 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index c28fe997..e4fc5ac7 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -876,7 +876,8 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> > int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > {
> > RPiCameraData *data = cameraData(camera);
> > - int count, ret;
> > + unsigned int index;
> > + int ret;
> >
> > /*
> > * Decide how many internal buffers to allocate. For now, simply look
> > @@ -896,30 +897,24 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > }
> >
> > /*
> > - * Add cookies to the ISP Input buffers so that we can link them with
> > - * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> > - */
> > - count = 0;
> > - for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> > - b->setCookie(count++);
> > - }
> > -
> > - /*
> > - * Add cookies to the stats and embedded data buffers and link them with
> > - * the IPA.
> > + * Link the FrameBuffers with the index of their position in the vector
> > + * stored in the RPi stream object.
> > + *
> > + * This will allow us to identify buffers passed between the pipeline
> > + * handler and the IPA.
> > */
> > - count = 0;
> > + index = 0;
> > for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> > - b->setCookie(count++);
> > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,
> > .planes = b->planes() });
> > + index++;
> > }
> >
> > - count = 0;
> > + index = 0;
> > for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> > - b->setCookie(count++);
> > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,
> > .planes = b->planes() });
> > + index++;
> > }
> >
> > data->ipa_->mapBuffers(data->ipaBuffers_);
> > @@ -1097,7 +1092,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> > unsigned int bufferId = action.data[0];
> > FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> >
> > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> > << ", timestamp: " << buffer->metadata().timestamp;
> >
> > isp_[Isp::Input].queueBuffer(buffer);
> > @@ -1117,12 +1112,14 @@ done:
> > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > {
> > RPi::RPiStream *stream = nullptr;
> > + int index;
> >
> > if (state_ == State::Stopped)
> > return;
> >
> > for (RPi::RPiStream &s : unicam_) {
> > - if (s.findFrameBuffer(buffer)) {
> > + index = s.getBufferIndex(buffer);
> > + if (index != -1) {
> > stream = &s;
> > break;
> > }
> > @@ -1132,7 +1129,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > ASSERT(stream);
> >
> > LOG(RPI, Debug) << "Stream " << stream->name() << " buffer dequeue"
> > - << ", buffer id " << buffer->cookie()
> > + << ", buffer id " << index
> > << ", timestamp: " << buffer->metadata().timestamp;
> >
> > if (stream == &unicam_[Unicam::Image]) {
> > @@ -1172,7 +1169,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> > return;
> >
> > LOG(RPI, Debug) << "Stream ISP Input buffer complete"
> > - << ", buffer id " << buffer->cookie()
> > + << ", buffer id " << unicam_[Unicam::Image].getBufferIndex(buffer)
> > << ", timestamp: " << buffer->metadata().timestamp;
> >
> > /* The ISP input buffer gets re-queued into Unicam. */
> > @@ -1183,12 +1180,14 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> > void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > {
> > RPi::RPiStream *stream = nullptr;
> > + int index;
> >
> > if (state_ == State::Stopped)
> > return;
> >
> > for (RPi::RPiStream &s : isp_) {
> > - if (s.findFrameBuffer(buffer)) {
> > + index = s.getBufferIndex(buffer);
> > + if (index != -1) {
> > stream = &s;
> > break;
> > }
> > @@ -1198,7 +1197,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > ASSERT(stream);
> >
> > LOG(RPI, Debug) << "Stream " << stream->name() << " buffer complete"
> > - << ", buffer id " << buffer->cookie()
> > + << ", buffer id " << index
> > << ", timestamp: " << buffer->metadata().timestamp;
> >
> > /*
> > @@ -1208,7 +1207,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > if (stream == &isp_[Isp::Stats]) {
> > IPAOperationData op;
> > op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> > - op.data = { RPiIpaMask::STATS | buffer->cookie() };
> > + op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> > ipa_->processEvent(op);
> > } else {
> > /* Any other ISP output can be handed back to the application now. */
> > @@ -1427,13 +1426,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);
> > +
> > LOG(RPI, Debug) << "Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:"
> > - << " Bayer buffer id: " << bayerBuffer->cookie()
> > - << " Embedded buffer id: " << embeddedBuffer->cookie();
> > + << " Bayer buffer id: " << bayerIndex
> > + << " Embedded buffer id: " << embeddedIndex;
> >
> > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedBuffer->cookie(),
> > - RPiIpaMask::BAYER_DATA | bayerBuffer->cookie() };
> > + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> > + RPiIpaMask::BAYER_DATA | bayerIndex };
> > ipa_->processEvent(op);
> > }
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index 6635a751..73eb04a1 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -178,15 +178,17 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)
> > }
> > }
> >
> > -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > +int RPiStream::getBufferIndex(FrameBuffer *buffer) const
> > {
> > if (importOnly_)
> > - return false;
> > + return -1;
> >
> > - if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> > - return true;
> > + /* 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);
> >
> > - return false;
> > + return -1;
> > }
> >
> > void RPiStream::clearBuffers()
> > @@ -199,7 +201,7 @@ void RPiStream::clearBuffers()
> >
> > int RPiStream::queueToDevice(FrameBuffer *buffer)
> > {
> > - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> > + LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferIndex(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 6222c867..45cf5237 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > @@ -48,7 +48,7 @@ public:
> > int queueAllBuffers();
> > int queueBuffer(FrameBuffer *buffer);
> > void returnBuffer(FrameBuffer *buffer);
> > - bool findFrameBuffer(FrameBuffer *buffer) const;
> > + int getBufferIndex(FrameBuffer *buffer) const;
> >
> > private:
> > void clearBuffers();
> >
>
> --
> Regards
> --
> Kieran
More information about the libcamera-devel
mailing list